GNOME Bugzilla – Bug 782949
matroskamux: reject H264 AVC on-the-fly codec_data changes (e.g. from rtph264depay)
Last modified: 2018-02-28 08:39:46 UTC
Hello, after updating latest changed related to rtph264depay I have noticed that the next commit makes a recorded video not to be played properly: commit 5394ec2f8ced475bf5567bf09af23781369fd0c9 Author: Tim-Philipp Müller <tim@centricular.com> Date: Mon Apr 24 17:29:37 2017 +0100 rtph264depay: don't insert SPS/PPS inline for AVC output SPS/PPS are in the caps in this case and shouldn't be in the stream data. If I do not misunderstand the patch, SPS/PPS info should be in the output caps of the rtph264depay, but the caps are the next: video/x-h264 stream-format: avc alignment: au codec_data: 0142c015ffe1000e6742c0... level: 2.1 profile: constrained-baseline Could I miss something to make it works with this patch?
The codec_data contains the SPS/PPS. What does not work exactly, how can it be reproduced?
Hello Sebastian, I think that the point that "stream-format: avc" is negotiated instead of "stream-format: byte-stream" setting the flag to FALSE [1]. This seems that matches the logic that Tim wanted to implement, but the stream is not correctly played. Does this change make developers to set the appropriate caps in order to use byte-stream or some fields in caps are missed in order to record the file properly? The worst point for me is that something that worked, has suddenly stopped to work with this patch :(. Refs [1] https://github.com/GStreamer/gst-plugins-good/blob/f13f3584acc129f33103165da485c1eabbd65895/gst/rtp/gstrtph264depay.c#L194
Can you provide some kind of testcase? With stream-format=avc, the SPS/PPS is only supposed to be signalled out-of-band (in the caps/codec_data), and should never change. If you need to support changing SPS/PPS (is that your problem?), you need to use byte-stream or avc3 (I think that's the one that has SPS/PPS in-band). Check the h264 spec for details.
It would help if you could describe what exactly worked before that doesn't work now? What muxer/container was used? I don't think the patch changes the format that gets negotiated?
Created attachment 352435 [details] GDP file with h264 RTP packets Hello Tim, a way to reproduce the problem I reported is generating a .mkv file using the attached GDP file with the next pipeline: gst-launch -v -m filesrc location=gdp-rtp-h264.out ! gdpdepay ! rtph264depay ! h264parse ! matroskamux ! filesink location=out.mkv If the patch I told you is used for the previous pipeline, out.mkv is not properly played but without this patch it works fine. out.mkv can be played with the next GStreamer pipeline or with ffplay. gst-launch -v -m filesrc location=out.mkv ! matroskademux ! avdec_h264 ! autovideosink I hope this helps you ;)
I believe the Matroska container does not support changing codec details (SPS/PPS) on the fly, at least not in the same stream. It may have worked before, but that's really more luck than by design. The file will not have worked everywhere. Use another container such as MPEG-TS where this is supported.
Hello again Tim, forcing "stream-format=byte-stream" like in [1] it works, so I would say that Matroska container does support it but in this way. The idea of forcing byte-stream is to pass the check added in your patch knowing that it worked before. Is this a proper way to make it to work or could it cause any problem? Refs [1] gst-launch -v -m filesrc location=gdp-rtp-h264.out ! gdpdepay ! rtph264depay ! "video/x-h264,stream-format=byte-stream" ! h264parse ! matroskamux ! filesink location=out.mkv
I'll have a look what's going on where exactly, but I don't think it should work. h264parse should signal changed caps when sps/pps changes, and matroskamux should probably reject those.
With your sample file, rtph264depay seems to correctly update the output caps with the new codec_data field when the SPS changes. However, matroskamux does not reject this on-the-fly caps change. I think it should.
And neither does mp4mux, hrm!
So is this a regression, what should happen here? :)
Well. There is a regression in behaviour of sorts, yes. As I see it, the commit and the resultant rtph264depay behaviour is entirely correct. Some may see this as a regression and may say "but it worked", but avc1 just doesn't support this on-the-fly change and the created files are somewhat broken. If in doubt I'd revert the commit in the 1.12 branch for now, until we've had time to update muxers to error out or do the right thing when caps change.
I assume there is a way to create a Matroska file where the SPS/PPS change? Maybe with multiple streams or something like that? but in the mean time, I'd just return an error on caps changes.
(In reply to Olivier Crête from comment #13) > I assume there is a way to create a Matroska file where the SPS/PPS change? > Maybe with multiple streams or something like that? You can't create a "timeline" in Matroska. But you could in theory append multiple Matroska files, just like chained Oggs. We don't support that yet though, see bug #334082. > but in the mean time, I'd just return an error on caps changes. Ack
Created attachment 368928 [details] [review] matroska-mux: Refuse caps changes after starting to write headers Matroska does not support changing the stream type and stream properties after the headers were started to be written, and for example H264 codec_data changes can't be supported.
Attachment 368928 [details] pushed as fc37bf7 - matroska-mux: Refuse caps changes after starting to write headers