GNOME Bugzilla – Bug 619293
[avimux] clean up avi header creation code
Last modified: 2010-07-09 13:23:18 UTC
Attached code uses enhanced bytewriter API (see bug #619292) to clean up avi header writing code (rather than all sorts of offset and size calculation voodoo).
Created attachment 161645 [details] [review] Clean up code for header writing using bytewriter
From the patch: + g_return_if_fail (gst_byte_writer_pop_mark (bw, &data, &pos, &size)); + Please do not ever put vital code into g_return_if_fail(), g_warn_if_fail() or g_assert(), as those are macros that can be disabled at compile time and cause bugs that are hard to find. It's really really important that this never happens. The currect way to write this is: + if (!gst_byte_writer_pop_mark (bw, &data, &pos, &size)) + g_return_if_reached(); + If you have done so before and reviews haven't caught it, please go back and check the elements. I've spent a really long time in a previous life going through all of these macros making sure they don't do bad stuff, which is why I'm very sensitive to this. ;)
Created attachment 161711 [details] [review] [avimux] clean up code for avi header using a bytewriter
So here's a reworked version that doesn't use the byte writer chunking API, but instead keeps chunks locally. The only thing this patch loses is the assertion check that indeed all chunks are popped in the end. On the other hand I like it a bit more, because it makes the chunks explicit in the code using them, so it's always clear which chunk we operate on, in particular when ending tags.
(In reply to comment #2) > If you have done so before and reviews haven't caught it, please go back and > check the elements. No, thanks. I do not make a habit of this. But this time around I specifically checked some GLib documentation which did not mention these macros could be compiled out (though they do mention so for g_assert), so I relaxed to go for it anyway.
The additional byte writer chunking API is hardly stunningly stellar, so it stands to reason it can be unwrapped and "do the job locally", with remaining taste in preference as minutia.
Created attachment 161920 [details] [review] Clean up code for header writing using bytewriter/chunkwriter Updated patch to using a GstChunkWriter.
In order to at least clean up avimux and not be stuck in API discussion, I suppose we might go for the plain approach for now.
commit 53365b91e6571d4b305664db85ef5ad4dc674848 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Sat May 22 11:55:37 2010 +0200 avimux: clean up code for avi header using a bytewriter
commit 53365b91e6571d4b305664db85ef5ad4dc674848 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Sat May 22 11:55:37 2010 +0200 avimux: clean up code for avi header using a bytewriter This commit is making camerabin tests crash in -bad.
Following commits should fix glitches in header rewrite. commit de5cb168ee30cbb06afca5a1cba624dfbd4ddde2 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Fri Jun 11 10:57:41 2010 +0200 avimux: fix avi header bytewriting ... by using proper offsets for tag list writing. Also use _reset rather than _free and consistently use bytewriter position. See #619293.
(In reply to comment #10) > commit 53365b91e6571d4b305664db85ef5ad4dc674848 > Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> > Date: Sat May 22 11:55:37 2010 +0200 > > avimux: clean up code for avi header using a bytewriter > > > This commit is making camerabin tests crash in -bad. Hmm, for me the tests still work.
(In reply to comment #12) > (In reply to comment #10) > > commit 53365b91e6571d4b305664db85ef5ad4dc674848 > > Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> > > Date: Sat May 22 11:55:37 2010 +0200 > > > > avimux: clean up code for avi header using a bytewriter > > > > > > This commit is making camerabin tests crash in -bad. > > Hmm, for me the tests still work. A fix has already been pushed. (See comment #11)