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 664443 - h264parse: Parsing shifts timestamps between frames
h264parse: Parsing shifts timestamps between frames
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
0.10.x
Other Mac OS
: Normal major
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-11-20 23:12 UTC by Matej Knopp
Modified: 2013-12-14 11:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to not always prepend the 0 to startcode (1.35 KB, patch)
2013-12-12 17:02 UTC, Matej Knopp
committed Details | Review

Description Matej Knopp 2011-11-20 23:12:22 UTC
Buffer for first frame ends with 0x00, 
Buffer for second frame start with 0x00 0x00 0x01 0x09

The following code in gsth264parser.c

/* sc might have 2 or 3 0-bytes */
  if (nalu->sc_offset > 0 && data[nalu->sc_offset - 1] == 00)
    nalu->sc_offset--;

assumes that the trailing zero byte from first frame is part of second frame start code. This results in gst_h264_parse_check_valid_frame returning fsize one byte smaller than it should be. Later fsize of bytes is taken from adapter, but since there is one last byte from first buffer, the timestamp for second frame is same as timestamp for first frame, which is wrong.

Removing the code fixes this but I don't know if it has any side effects.
Comment 1 Edward Hervey 2013-07-24 07:32:02 UTC
Matej, is this still an issue in 1.x/git ? h264parse changed a lot since 2011.
Comment 2 Matej Knopp 2013-07-24 13:11:22 UTC
Yeah, I don't even remember how to reproduce this. I'll close it as obsolete and reopen it if needed.
Comment 3 Matej Knopp 2013-12-12 16:59:14 UTC
So this is still an issue. The parser assumes that every time there is a 0 before the startcode, it is part of the startcode. But that's not true

From the specification

Byte stream NAL unit syntax
zero_byte is a single byte equal to 0x00.
  When any of the following conditions are fulfilled, the zero_byte syntax element shall be present.
– the nal_unit_type within the nal_unit( ) is equal to 7 (sequence parameter set) or 8 (picture parameter set)
  – the byte stream NAL unit syntax structure contains the first NAL unit of an access unit in decoding order, as specified by subclause 7.4.1.2.3.

The problem with doing this for all startcodes is that a trailing zero can mess up timestamps. The trailing zero gets prepended to the startcode, which will carry  the PTS and DTS of previous buffer.
Comment 4 Matej Knopp 2013-12-12 17:02:11 UTC
Created attachment 264090 [details] [review]
Patch to not always prepend the 0 to startcode

Simple patch - it doesn't cover all cases from 7.4.1.2.3 - that would require much more involved parsing and lot more state in nal scanner. Still, omitting the zero byte in some cases is probably better than taking the zero from previous buffer (and thus messing up timestamps)
Comment 5 Sebastian Dröge (slomo) 2013-12-14 11:17:56 UTC
commit 6af387cd5ab2c946025e5499903e75ee87b063a9
Author: Matej Knopp <matej.knopp@gmail.com>
Date:   Thu Dec 12 17:49:24 2013 +0100

    h264parser: not all startcodes should have 3-byte 0 prefix
    
    The parser assumes that every time there is a 0 before the startcode,
    it is part of the startcode. But that's not true.
    
    From the specification
    
    Byte stream NAL unit syntax
    zero_byte is a single byte equal to 0x00.
      When any of the following conditions are fulfilled, the zero_byte syntax
      element shall be present.
      – the nal_unit_type within the nal_unit( ) is equal to 7 (sequence parameter
        set) or 8 (picture parameter set)
      – the byte stream NAL unit syntax structure contains the first NAL unit of an
        access unit in decoding order, as specified by subclause 7.4.1.2.3.
    
    The problem with doing this for all startcodes is that a trailing zero can mess
    up timestamps. The trailing zero gets prepended to the startcode, which will
    carry the PTS and DTS of previous buffer.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=664443