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 739345 - codecparsers: mpeg4: fix ignored increment of return value
codecparsers: mpeg4: fix ignored increment of return value
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal minor
: 1.5.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-10-29 10:44 UTC by Luis de Bethencourt
Modified: 2015-06-13 18:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
remove the increment (962 bytes, patch)
2014-10-29 10:45 UTC, Luis de Bethencourt
rejected Details | Review
Do the increment correctly and clean the code (2.76 KB, patch)
2014-10-29 15:10 UTC, Luis de Bethencourt
none Details | Review
Correctly increment the return value (2.21 KB, patch)
2014-10-29 16:41 UTC, Luis de Bethencourt
committed Details | Review

Description Luis de Bethencourt 2014-10-29 10:44:24 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.
Comment 1 Luis de Bethencourt 2014-10-29 10:45:46 UTC
Created attachment 289573 [details] [review]
remove the increment

Patch to remove the increment
Comment 2 Luis de Bethencourt 2014-10-29 10:48:22 UTC
Tested this.
Comment 3 Luis de Bethencourt 2014-10-29 10:49:03 UTC
Comment on attachment 289573 [details] [review]
remove the increment

Merged.
Comment 4 Sebastian Dröge (slomo) 2014-10-29 11:10:01 UTC
The comment seems to hint that "return ++val" was intended here
Comment 5 Luis de Bethencourt 2014-10-29 14:52:09 UTC
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.
Comment 6 Luis de Bethencourt 2014-10-29 15:10:41 UTC
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.
Comment 7 Luis de Bethencourt 2014-10-29 16:41:44 UTC
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.
Comment 8 Luis de Bethencourt 2014-10-29 17:45:15 UTC
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 :)
Comment 9 Luis de Bethencourt 2014-10-29 17:47:32 UTC
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
Comment 10 Tim-Philipp Müller 2015-06-13 17:59:20 UTC
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 11 Tim-Philipp Müller 2015-06-13 18:00:38 UTC
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.