Hi, not sure who to send this to, since it seems to be duplicated in both speex and liboggz, so I'm sending to the whole list. This fixes an off by one bug in the user of snprintf, and returns negative if writing the header returns negative (otherwise we'd just get a short write, losing the error). This patch isn't tested though, but I compiled it :) Also, I've not fixed this (yet ?) but I believe the filling of the ogg packets is bogus as it will fill it with values in whatever endianness the current host has. It should be using oggpack_write and friends instead, I think. I may be missing something though. I found this while having a look at liboggz, which Ivo pointed me to. This seems to do what ogg-tools do, only more and better, from the admittedly quick read I had of it.. Are there plans to use these rather than ogg-tools as the "official" way of manipulating ogg streams ? -------------- next part -------------- A non-text attachment was scrubbed... Name: skeleton.c.diff Type: text/x-patch Size: 1310 bytes Desc: not available Url : http://lists.xiph.org/pipermail/ogg-dev/attachments/20080131/3659fee9/skeleton.c.bin
On 31/01/2008, ogg.k.ogg.k@googlemail.com <ogg.k.ogg.k@googlemail.com> wrote:> Hi, > > not sure who to send this to, since it seems to be duplicated in both > speex and liboggz, > so I'm sending to the whole list.thanks, this is an appropriate place for that patch. That skeleton.c comes from a Summer of Code project which put skeleton support into many projects; we'll need to apply this patch in those too.> This fixes an off by one bug in the user of snprintf, and returns > negative if writing the > header returns negative (otherwise we'd just get a short write, losing > the error). > This patch isn't tested though, but I compiled it :)ok, I'll test it out :-)> Also, I've not fixed this (yet ?) but I believe the filling of the ogg > packets is bogus as it > will fill it with values in whatever endianness the current host has. > It should be using > oggpack_write and friends instead, I think. I may be missing something though.eww, you're right, it's broken on big-endian hosts. I'll follow up with a patch unless someone else does first ...> I found this while having a look at liboggz, which Ivo pointed me to. > This seems to do > what ogg-tools do, only more and better, from the admittedly quick > read I had of it.. > Are there plans to use these rather than ogg-tools as the "official" > way of manipulating > ogg streams ?there's been a lot of contributions to oggz lately, more welcome too :-) I guess the only reason we have separate projects for ogg-tools and oggz-tools is library dependencies. cheers, Conrad.
I'm patching oggenc with this fix. If Conrad reports success in his test, I'll go ahead and patch all other implementations I can find. -Ivo
On 01/02/2008, Conrad Parker <conrad@metadecks.org> wrote:> On 31/01/2008, ogg.k.ogg.k@googlemail.com <ogg.k.ogg.k@googlemail.com> wrote: > > This fixes an off by one bug in the user of snprintf, and returns > > negative if writing the > > header returns negative (otherwise we'd just get a short write, losing > > the error). > > This patch isn't tested though, but I compiled it :) > > ok, I'll test it out :-)I've checked the patch. The check for fwrite returning negative is ok, but i think the off-by-one check for snprintf is not necessary -- the intention is not to write a trailing NUL as this function is used to incrementally add lines of text to a buffer. Adapted in changeset 3396: http://trac.annodex.net/changeset/3396> > Also, I've not fixed this (yet ?) but I believe the filling of the ogg > > packets is bogus as it > > will fill it with values in whatever endianness the current host has. > > It should be using > > oggpack_write and friends instead, I think. I may be missing something though. > > eww, you're right, it's broken on big-endian hosts. I'll follow up > with a patch unless someone else does first ...I've written up a patch for this, applied to liboggz/src/tools/skeleton.c: http://trac.annodex.net/changeset/3395 Thank you very much for noticing the problems :-) Conrad.