[Bug 739345] codecparsers: mpeg4: fix ignored increment of return value
GStreamer (GNOME Bugzilla)
bugzilla at gnome.org
Sat Jun 13 10:59:20 PDT 2015
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.
More information about the gstreamer-bugs
mailing list