GNOME Bugzilla – Bug 758656
h264parse: failed to encode MVC h264 streams in GStreamer-1.6
Last modified: 2018-11-03 13:43:33 UTC
MVC encoding is failing when adding a videoparser element in the pipeline. Seems to be a regression with recent MVC specific patches landed in upstream h264parser (>= 1.5). Encode(run with out any error): GST_DEBUG=3 gst-launch-1.0 videotestsrc num-buffers=900 ! vaapiencode_h264 num-views=2 ! h264parse ! filesink location=enc_mvc.h264 Decoding will fail: GST_DEBUG=3 gst-launch-1.0 filesrc location= enc_mvc.h264 ! h264parse ! "video/x-h264, stream-format=(string)byte-stream" ! vaapidecode ! vaapisink
This is a regression in h264parser introduced by this commit: commit e8908f5aeef952566f6bccde743c7735d3f8c6ef Author: Jan Schmidt <jan@centricular.com> Date: Thu Jun 11 01:57:08 2015 +1000 h264parse: Don't switch to passthrough on set_caps() Wait until at least one keyframe has been parsed before deciding to switch to passthrough mode, in case the stream contains SEI messages that supplement the output caps - for example by providing stereoscopic information The logic to set passthrough mode in the pre_push_frame() based on "have_keyframe && have_sps && have_pps" is wrong, since MVC streams may come up as two layers(base-layer and non-base layer). In this case first sps, first pps, first slice_hdr and first keyframe data should get parsed as base-layer and then only we will receive subset_sps, pps, slice_hdr of non base-layer.
Assigning to upstream..
Created attachment 316332 [details] [review] videoparsers: h264: Disable passthorugh mode enabling This is a quick fix I did for immediate gstreamer-vaapi-0.7.0 release. The patch is disabling the passthrough mode, otherwise it will simply break multi-layer mvc stream parsing. I am not sure whether it is acceptable for upstream to just disable it. But IHMO it is better to disable it until we fix all issues, otherwise it will make streams unplayable. Also I Have seen other places too where MVC handling requires proper fix. I will get back to this later unless someone else fix all the issues :)
BTW, Setting passthroug mode can cause issues for non-mvc streams too :) For eg: consider a multi-resolution elementary nal alignend byte-stream which has multiple sps headers where each contains different resolution. We should parse all the headers and set the source caps in this case. Passthrough mode make it impossible, right? Am I missing something here?
Comment on attachment 316332 [details] [review] videoparsers: h264: Disable passthorugh mode enabling This patch doesn't look like a proper fix to me. My reading of Jan's commit is that it just delays the activation of passthrough mode, so not sure if it should've broken something that wasn't broken already before, but I didn't investigate in detail. We need more unit tests for h264parse :)
In general, I think the h264parse passthrough is over-eager. It does need to be turned off, or made to still do some minimal inspection of the stream or something to handle these more complex cases properly.
(In reply to Tim-Philipp Müller from comment #5) > Comment on attachment 316332 [details] [review] [review] > videoparsers: h264: Disable passthorugh mode enabling > > This patch doesn't look like a proper fix to me. > > My reading of Jan's commit is that it just delays the activation of > passthrough mode, so not sure if it should've broken something that wasn't > broken already before, but I didn't investigate in detail. > > We need more unit tests for h264parse :) Aha, I almost forgot about this, was using vaapiparse_h264 always :) This is indeed a regression, you can test with what I mentioned in comment-0. I will get back to this soon, I would like to get this fixed for 1.8 release (when it will be?), otherwise MVC encoding with gstreamer-vaapi is not going to work ... Prefer to raise the severity...
Did it ever work with h264parse? If you want this for 1.8.0 you'll have to hurry a bit, 1.7.90 will be today or so already. But if this is just going to be a bugfix it can go into 1.8.1 later.
(In reply to Sebastian Dröge (slomo) from comment #8) > Did it ever work with h264parse? If you want this for 1.8.0 you'll have to > hurry a bit, 1.7.90 will be today or so already. But if this is just going > to be a bugfix it can go into 1.8.1 later. It will work if you revert e8908f5aeef952566f6bccde743c7735d3f8c6ef.. But even with out any further investigation I prefer to disable the passthrough mode. See comment-4
(In reply to Sebastian Dröge (slomo) from comment #8) > Did it ever work with h264parse? If you want this for 1.8.0 you'll have to > hurry a bit, 1.7.90 will be today or so already. But if this is just going > to be a bugfix it can go into 1.8.1 later. Without fixing this, the pass rate of gstreamer-vaapi mvc encoding is going to be zero with 1.8 which was 100% with gstreamer-vaapi-0.7.0 :)
Ok. Disabling passthrough mode completely is just a workaround though, not a fix. Can you analyse the problem here and work on a proper fix?
(In reply to Sebastian Dröge (slomo) from comment #11) > Ok. Disabling passthrough mode completely is just a workaround though, not a > fix. Can you analyse the problem here and work on a proper fix? Had a quick analysis. Once we set pass-through mode, the only way to get the multi-resolution videos to work properly is: do the nal header parsing in pre_push_frame() to check whether there is a new sps or not. which is in theory parsing again...
Created attachment 322450 [details] multi-resolution sample for testing: gst-launch-1.0 -v filesrc location= multiresolution_rottest.ts ! tsdemux ! h264parse ! "video/x-h264, stream-format=byte-stream, alignment=nal" ! fakesink We only get one src caps in the entire playback from h264 which is h264parse0.GstPad:src: caps = "video/x-h264\,\ stream-format\=\(string\)byte-stream\,\ alignment\=\(string\)nal\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ width\=\(int\)320\,\ height\=\(int\)240\,\ framerate\=\(fraction\)30/1\,\ parsed\=\(boolean\)true\,\ profile\=\(string\)constrained-baseline\,\ level\=\(string\)1.3" We never get a src caps update from parser for any resolution change because of passthrough
Again, better disable the passthrough :)
Why don't we know in pre_push_frame() if there is a new SPS or not? Shouldn't it be the first NAL in that buffer anyway? Also we can possibly do something with buffer flags here, remember from earlier if there was an SPS or not?
(In reply to Sebastian Dröge (slomo) from comment #15) > Why don't we know in pre_push_frame() if there is a new SPS or not? Because once we set passthrough initially, we never again try to parse incoming stream,,just push the buffers as it is. If we have to know whether there is an sps or not, we need to parse the buffer again.. > Shouldn't it be the first NAL in that buffer anyway? AFAIK, if there is an sps, it could be the first one in a buffer.. But we don't know which buffer has this sps... >Also we can possibly do > something with buffer flags here, remember from earlier if there was an SPS > or not? NO, once we set passthrough, we have no earlier parsing too...
Why do we need to do anything at all if we go into passthrough mode here? Maybe we just shouldn't go into passthrough for MVC if parsing is actually needed to do the right thing for MVC?
(In reply to Sebastian Dröge (slomo) from comment #17) > Why do we need to do anything at all if we go into passthrough mode here? > Maybe we just shouldn't go into passthrough for MVC if parsing is actually > needed to do the right thing for MVC? comment 13 is general case..not mvc. For me issue was with mvc, but it seems the passthrough is wrong even for non-mvc cases..
Then we should disable passthrough in general :)
I mean, if we always need to parse there's no point in ever doing passthrough... as we always need to parse.
I may be misremembering, it's been a while, but I think the problem was just that it switches to passthrough too early and therefore fails to detect and set the right profile/level/view info into the output caps. It only switches to passthrough when the data is already suitably packetised, just too soon. On the other hand, once it does switch to passthrough, it'll never spot changes if there are any. The problem with just turning off passthrough is extra CPU consumption. Maybe there's some middle ground for already-packetised streams, where it just inspects the packets with lower overhead (requires significant patch I guess)
Enabling passthrough only for avc streams , and no passthrough at all for byte-stream could be possible
So whats the conclusion, waiting for patch/disable it/sommething else ?? :)
If the problem is just that we go to passthrough too early, then that should be fixed. Otherwise disabling for 1.8 and implementing Jan's idea of similified parsing later seems best.
Problem is *not* just "whether we enable passthrough early or not"...If we need h264parser to advertise the exact resolution for each change, then disable-passthrough (attached patch can still be used then)... Of course optimizations possible, but i prefer disabling for now (unless Jan is working on different patch)...
(In reply to sreerenj from comment #25) > Of course optimizations possible, but i prefer disabling for now (unless Jan > is working on different patch)... I'm not - not in the next few days at least.
commit 8789b89f77c94d00610747d70c406032e4ee0b2b Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Mon Feb 29 12:19:51 2016 +0200 videoparsers: h264: Disable passthorugh mode enabling Enabling passthorugh mode is causing multiple issue: For nal aligned multiresoluton streams, passthrough mode make h264parse unable to advertise the new resoultions. Also causing issues while parsing MVC streams which have two separate layers (base-view and non-base-view). This fix is only a temporary workaroud. For MVC, proper fixes needed in many places: (handle prefix nal unit, handle non-base-view slice nal extension, fix the picture_start detection for multi-layer-mvc streams etc) https://bugzilla.gnome.org/show_bug.cgi?id=758656 Disabled passthrough mode for now,,, Keeping bug as open since we need better optimization.
Created attachment 322644 [details] [review] videoparsers: h264: Disable passthorugh mode enabling
Review of attachment 322644 [details] [review]: Pushed.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/330.