GNOME Bugzilla – Bug 739345
codecparsers: mpeg4: fix ignored increment of return value
Last modified: 2015-06-13 18:01:04 UTC
in gst-libs/gst/codecparsers/gstmpeg4parser.c:350 > return val++; The value is returned before it is incremented because the post increment happens after the statement.
Created attachment 289573 [details] [review] remove the increment Patch to remove the increment
Tested this.
Comment on attachment 289573 [details] [review] remove the increment Merged.
The comment seems to hint that "return ++val" was intended here
Sebastian, I noticed that but the code works fine without needing ++val. I tested it. Plus I checked git blame and it has been like this for a long time without issues. Will investigate a bit more though.
Created attachment 289595 [details] [review] Do the increment correctly and clean the code The problem was that the code was adding 1 just before the only usage of markersize. > if (gst_bit_reader_get_bits_uint32_unchecked (&br, markersize + 1) != 0x01) So to make this cleaner I brought the increment back, but as a pre-increment and removed the + 1 in the above line. Also, the variable markersize doesn't need to be stored in the GstMpeg4Packet structure since the above is the only function block that uses it. So I removed that and cleaned it.
Created attachment 289598 [details] [review] Correctly increment the return value Fixing the previous patch that broke API. Correctly increment the return value, instead of incrementing it at usage.
Hahahahahaha... that code never actually runs gst-libs/gst/codecparsers/gstmpeg4parser.c:424 > GstMpeg4ParseResult > gst_mpeg4_parse (GstMpeg4Packet * packet, gboolean skip_user_data, > GstMpeg4VideoObjectPlane * vop, const guint8 * data, guint offset, > gsize size) > { Every single instance that calls that function in the actual code passes NULL for the vop object parameter. and then: > if (vop) { > resync_res = > gst_mpeg4_next_resync (packet, vop, data + offset, size - offset, > first_resync_marker); gst_mpeg4_next_resync, which only runs if vop is !NULL (which never is) is the usage of compute_resync_marker_size() that actually uses the returned value. This is nice to have implemented, but as it stands, nobody ever uses it. Looking at the bitstream it looks ++off was the intended thing since the returned value off is the equivalent of "space until the next marker, and one more to be *AT* the beginning of the next packet". But nobody ever catched the bug because it never runs :)
Forgot to say on the above comment. If we want to keep the intended behaviour, I am proposing to apply patch 289598. https://bug739345.bugzilla-attachments.gnome.org/attachment.cgi?id=289598
I'm not really sure what to do with this. On the one hand I'd say don't fix it if it ain't broken. But then it does seem like it should be +1, the resync marker is N zero bits plus one 1 bit, and taking into account the pattern/mask, it should really return the full number of bits IMHO. Also, while the code in gst_mpeg4_parse_video_packet_header() does the +1 when calling get_bits_uint32_unchecked(), it doesn't actually do the +1 in the CHECK_REMAINING, so we might not have enough bits here actually (theoretically, anyway). (In reply to Luis de Bethencourt from comment #8) > Hahahahahaha... that code never actually runs > > gst-libs/gst/codecparsers/gstmpeg4parser.c:424 > > GstMpeg4ParseResult > > gst_mpeg4_parse (GstMpeg4Packet * packet, gboolean skip_user_data, > > GstMpeg4VideoObjectPlane * vop, const guint8 * data, guint offset, > > gsize size) > > { > > Every single instance that calls that function in the actual code passes > NULL for the vop object parameter. (snip) > > This is nice to have implemented, but as it stands, nobody ever uses it. > > Looking at the bitstream it looks ++off was the intended thing since the > returned value off is the equivalent of "space until the next marker, and > one more to be *AT* the beginning of the next packet". But nobody ever > catched the bug because it never runs :) Note that this is public API, and external code might use this function and call it with a non-NULL vop. And some actually does (e.g. gstreamer-vaapi). Anyway, pushed this now, fingers crossed: commit 417db39d0d61b8944d9ddd7f7f8f836c58260206 Author: Luis de Bethencourt <luis.bg@samsung.com> Date: Wed Oct 29 15:03:04 2014 +0000 codecparsers: mpeg4: actually return full number of bits of resync marker Switch the increment of markersize from when it is used to when it is returned from compute_resync_marker_size. This also makes the CHECK_REMAINING in gst_mpeg4_parse_video_packet_header check for the actually required number of bits now and not one too few. https://bugzilla.gnome.org/show_bug.cgi?id=739345 commit b14fb383eddec5aa086524ba39ade684b29bd217 Author: Tim-Philipp Müller <tim@centricular.com> Date: Sat Jun 13 17:36:20 2015 +0100 Revert "codecparsers: remove ignored increment of return" This reverts commit 916b954315abc2f94348ec0be3e116c19b080b54. Clearly something else was intended, and it also makes more sense to add the extra bit. The resync marker is N zero bits plus a 1 bit, and the pattern/mask needs to be run on N+1 bits too. (Even after the rever the code doesn't do that of course, so it still needs to be fixed differently.) https://bugzilla.gnome.org/show_bug.cgi?id=739345
Comment on attachment 289598 [details] [review] Correctly increment the return value Committed this with changes: return ++off -> return off + 1; (KISS) skipped the gratuitious changes with the markersize variable (I think we only want to fill in the value in the packet structure if we return OK). Last chunk is ok.