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 705333 - h264parse : broken bytestream/nal => avc/au conversion
h264parse : broken bytestream/nal => avc/au conversion
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-libav
git master
Other Linux
: Normal blocker
: 1.1.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-08-02 09:06 UTC by Alexander E. Patrakov
Modified: 2013-09-09 13:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
h264parse: Do not trigger caps update if we only have PPS updates (1.53 KB, patch)
2013-08-03 12:23 UTC, Edward Hervey
committed Details | Review
Patch (2.44 KB, patch)
2013-09-07 02:06 UTC, Matej Knopp
none Details | Review
h264parse: don't update src caps if only codec_data differs (2.49 KB, patch)
2013-09-07 21:11 UTC, Matej Knopp
committed Details | Review

Description Alexander E. Patrakov 2013-08-02 09:06:23 UTC
The attached file is a beginning of a BluRay rip. It plays fine in mplayer and even with gstreamer-0.10. However, if I run the following command, the playback is very jerky and flashy, with a lot of artifacts:

gst-launch-1.0 playbin uri=file:///home/aep/1.m2ts

If I rebuild gst-libav against ffmpeg-1.2.1 instead of the included libav, artifacts and flashes disappear, but the playback is not smooth. So it is still a bug.
Comment 1 Alexander E. Patrakov 2013-08-02 09:17:48 UTC
Bugzilla told me that the file is too large. Get it from this ad-infested download page: http://www.zalil.ru/34656947

Note: it will be deleted automatically in 10 days.
Comment 2 Edward Hervey 2013-08-02 10:04:15 UTC
this is due to this issue : https://bugzilla.gnome.org/show_bug.cgi?id=702327
Comment 3 Edward Hervey 2013-08-03 08:32:53 UTC
The core issue is that the bytestream/au to avc/nal conversion is broken in h264parse for some streams.
Comment 4 Edward Hervey 2013-08-03 08:34:59 UTC
*cough* the other conversion :)
Comment 5 Edward Hervey 2013-08-03 10:06:17 UTC
And it's a bit trickier in fact

The NALU received start with
AU_DELIMITER / SPS / PPS / SEI / Slice....

And then repeats of:
AU_DELIMITER / PPS / SEI / Slice .... 

And then back to the first sequence (with SPS and PPS)

So essentially the PPS is constantly sent before each frame ... and it might change.

When that PPS changes, we send caps with new codec_data ... which end up resetting the decoder ...

The problem is that the decoder will be starting decoding again from the middle of a group of pictures (i.e. a non-reference Slice) and output garbage.

Maybe we should only update the codec_data is the *SPS* changed (and not the PPS which will be sent anyway in stream).
Comment 6 Olivier Crête 2013-08-03 11:39:43 UTC
In theory you could also have multiple SPSes I think, and I guess they could change at any point. The decoder should probably not reset its state on a new caps event if none of the important parameters have changed.
Comment 7 Edward Hervey 2013-08-03 12:19:31 UTC
We wouldn't be dropping the PPS fwiw, it's just that we wouldn't *update* the codec_data if it's only a SPS update/addition.
Comment 8 Edward Hervey 2013-08-03 12:23:53 UTC
Created attachment 250768 [details] [review]
h264parse: Do not trigger caps update if we only have PPS updates

Updating caps results in downstream elements potentially reconfiguring themselves
(such as decoders). If we do this in the middle of keyframes, we would result
in those elements being reconfigured and handling garbage until the next keyframe.

Instead of this only send (potentially) new codec_data when we have *both* SPS and
PPS.
Comment 9 Edward Hervey 2013-08-04 10:09:50 UTC
commit b17676a1d5f1f9179eef32b92b22ecfbce902112
Author: Edward Hervey <edward@collabora.com>
Date:   Sat Aug 3 14:20:47 2013 +0200

    h264parse: Do not trigger caps update if we only have PPS updates
    
    Updating caps results in downstream elements potentially reconfiguring themselves
    (such as decoders). If we do this in the middle of keyframes, we would result
    in those elements being reconfigured and handling garbage until the next keyframe.
    
    Instead of this only send (potentially) new codec_data when we have *both* SPS and
    PPS.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=705333
Comment 10 Matej Knopp 2013-09-07 01:54:14 UTC
The patch is not enough. The PPS replaces PPS in parser, so next codec_data will be different. On next SPS update_caps is set to true, which will build codec data and compare it to current codec data, resulting in CAPS  event being sent. The caps event has all properties same except the codec data. 

I think the proper solution here would be to not update caps if it only differs in codec_data.
Comment 11 Matej Knopp 2013-09-07 02:06:44 UTC
Created attachment 254326 [details] [review]
Patch

Something like this would help I think.
Comment 12 Matej Knopp 2013-09-07 21:11:48 UTC
Created attachment 254360 [details] [review]
h264parse: don't update src caps if only codec_data differs

(forgot to add check in previous version)
Comment 13 Olivier Crête 2013-09-08 14:27:42 UTC
You have to send downstream updated codec_data, otherwise the decoder can't decode the next frames.
Comment 14 Matej Knopp 2013-09-08 14:31:57 UTC
Updated SPS and PPS are always present in the bitstream. The decoder will pick them from there. Sending them in caps means you are reopening the decoder every time SPS/PPS changes, which is most certainly not what you want to do.
Comment 15 Sebastian Dröge (slomo) 2013-09-09 13:10:39 UTC
Comment on attachment 254360 [details] [review]
h264parse: don't update src caps if only codec_data differs

commit a41e8698b1ae62b18fd2449dfcb7c1b9d39d02b9
Author: Matej Knopp <matej.knopp@gmail.com>
Date:   Sat Sep 7 23:09:31 2013 +0200

    h264parse: don't update src caps if only codec_data differs
    
    https://bugzilla.gnome.org/show_bug.cgi?id=705333