After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 619293 - [avimux] clean up avi header creation code
[avimux] clean up avi header creation code
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 0.10.24
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 619292
Blocks:
 
 
Reported: 2010-05-21 13:39 UTC by Mark Nauwelaerts
Modified: 2010-07-09 13:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Clean up code for header writing using bytewriter (23.90 KB, patch)
2010-05-21 13:40 UTC, Mark Nauwelaerts
none Details | Review
[avimux] clean up code for avi header using a bytewriter (24.16 KB, patch)
2010-05-22 09:58 UTC, Benjamin Otte (Company)
committed Details | Review
Clean up code for header writing using bytewriter/chunkwriter (24.21 KB, patch)
2010-05-25 08:54 UTC, Mark Nauwelaerts
none Details | Review

Description Mark Nauwelaerts 2010-05-21 13:39:35 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).
Comment 1 Mark Nauwelaerts 2010-05-21 13:40:11 UTC
Created attachment 161645 [details] [review]
Clean up code for header writing using bytewriter
Comment 2 Benjamin Otte (Company) 2010-05-22 09:25:26 UTC
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. ;)
Comment 3 Benjamin Otte (Company) 2010-05-22 09:58:35 UTC
Created attachment 161711 [details] [review]
[avimux] clean up code for avi header using a bytewriter
Comment 4 Benjamin Otte (Company) 2010-05-22 10:01:34 UTC
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.
Comment 5 Mark Nauwelaerts 2010-05-22 19:46:07 UTC
(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.
Comment 6 Mark Nauwelaerts 2010-05-22 19:58:10 UTC
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.
Comment 7 Mark Nauwelaerts 2010-05-25 08:54:49 UTC
Created attachment 161920 [details] [review]
Clean up code for header writing using bytewriter/chunkwriter

Updated patch to using a GstChunkWriter.
Comment 8 Mark Nauwelaerts 2010-06-02 11:30:47 UTC
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.
Comment 9 Benjamin Otte (Company) 2010-06-02 12:24:06 UTC
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
Comment 10 Thiago Sousa Santos 2010-06-10 20:14:55 UTC
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.
Comment 11 Mark Nauwelaerts 2010-06-11 09:01:11 UTC
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.
Comment 12 Stefan Sauer (gstreamer, gtkdoc dev) 2010-07-09 13:09:02 UTC
(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.
Comment 13 Thiago Sousa Santos 2010-07-09 13:23:18 UTC
(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)