[Bug 739345] codecparsers: mpeg4: fix ignored increment of return value

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=739345

Tim-Philipp Müller <t.i.m at zen.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|1.5.1                       |1.5.2
            Summary|codecparsers: remove        |codecparsers: mpeg4: fix
                   |ignored increment of return |ignored increment of return
                   |                            |value

--- Comment #10 from Tim-Philipp Müller <t.i.m at zen.co.uk> ---
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 at 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 at 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

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.