Created attachment 246882 [details] [review]
h264parse: Fixate to upstream format when possible

This breaks the parser support in decodebin. Will provide details later

discussion with sebastian about this issue

<slomo> bilboed: basically decodebin uses a capsfilter after parsers to allow it to negotiate some stream format that is accepted by decoders... this capsfilter contains first the highest ranked decoder's sinkpad caps, then the next highest ranked, ... and in the end the src template caps of the parser (to prevent negotiation to fail and get negotiation error instead of "missing plugin")
<slomo> bilboed: this is done because the parser has downstream not linked before it doesn't set the srcpad caps. and it can't set the correct srcpad caps before it doesn't know what stream format downstream wants
<slomo> bilboed: now if you prefer passthrough... you will always passthrough
<slomo> bilboed: maybe now that we can intercept queries properly in decodebin this capsfilter hack can be removed... and instead the CAPS query can be answered for the parser, not including the parser srcpad template caps
<bilboed> it should reconfigure itself when it gets RECONFIGURED (which should happen when it's actually linked), no ?
<bilboed> I didn't know about that capsfilter hack though
<bilboed> and don't really understand its purpose
<slomo> it is done because the parser has downstream not linked before it doesn't set the srcpad caps. and it can't set the correct srcpad caps before it doesn't know what stream format downstream wants
<slomo> and yes, h264parse should renegotiate. but doing that without losing data is not trivial and not implemented either
<bilboed> (before it doesn't set ? what does that mean ?)
<slomo> decodebin only plugs the decoder after the parser after the parser has sent the CAPS event
<bilboed> ... *sigh*
<slomo> it's the same problem over and over again in different situations :)
<bilboed> well... my point is that with byte-stream we never do passthrough
<slomo> why not?
<bilboed> because in the fixation ... it always ends up fixating stream-format to the first choice ("avc")
<bilboed> maybe the parser source pad caps query should return those fields sorted by "upstream-first" ?
<bilboed> or wouldn't that make a differenc e?
<bilboed> I guess it wouldn't :(
<slomo> but if downstream only can do bytestream, it will do passthrough, right?
<bilboed> yes, but some decoders support both
<bilboed> maybe the parser source pad caps query should return those fields sorted by "upstream-first" ?
<bilboed> or wouldn't that make a differenc e?
<bilboed> I guess it wouldn't :(
<slomo> but if downstream only can do bytestream, it will do passthrough, right?
<bilboed> yes, but some decoders support both
<slomo> only if downstream can handle both (in the same structure!) it will prefer avc over passthrough... but that is something you could handled specifically in the parser negotiation
<bilboed> gst-inspect-1.0 avdec_h264
<bilboed> you'll see what I mean
<slomo> so you don't do the normal fixation stuff... but if the first structure contains an array for the stream-format, you chose passthrough
<slomo> it's all not ideal though... and especially if you have a hardware decoder that can do bytestream ranked highest, and avdec_h264 after that, you really don't want h264parse to negotiate to avc. as then decodebin will not even try to use the hardware decoder

I think for the long term we should really just remove this capsfilter hack and intercept the caps query. See what happens in autoplug_query already.

Would need to get a default implementation in decodebin, and another implementation in uridecodebin that is equivalent... and yet another one in playbin that uses the different autoplugging preferences there (which consider the caps features and sinks and magic).

Additionally h264parse should be more clever with fixation of the stream-format field. If the first structure in the caps allows both stream-formats, it should prefer passthrough. But it should only consider the first structure (that intersects with the actual caps we can produce, considering profile/level/etc).

Also, current this pipeline doesn't work because the downstream caps are ignored, and the framerate is not taken into account..


gst-launch-1.0 filesrc location=h264-bytestream-file ! h264parse ! video/x-h264, framerate=30/1 ! qtmux ! fakesink

Created attachment 251695 [details] [review]
Carefully choose stream-format for h264parse src pad

If caps "intersect" fails, then rather than fixating to "avc" I am choosing appropriate stream-format for h264parse src caps based on the downstream elements stream-format.

Review of attachment 251695 [details] [review]:

::: code/Gstreamer/gstreamer-1.0.8/gsth264parse.c
@@ +350,3 @@
+	const char *ai1_stream_format[3] = {"unknown", "avc", "byte-stream"};
+
+	src_caps_st = gst_caps_get_structure (caps, 0);

The caps can have multiple structures here

AFAIU you could just loop over all structures, and for each structure call gst_structure_fixate_field_string(s, "stream-format", upstream_stream_format). That will chose the upstream stream format if possible, otherwise chose one of the possible ones.

(In reply to comment #8)
> Review of attachment 251695 [details] [review]:
> 
> ::: code/Gstreamer/gstreamer-1.0.8/gsth264parse.c
> @@ +350,3 @@
> +    const char *ai1_stream_format[3] = {"unknown", "avc", "byte-stream"};
> +
> +    src_caps_st = gst_caps_get_structure (caps, 0);
> 
> The caps can have multiple structures here
> 
> AFAIU you could just loop over all structures, and for each structure call
> gst_structure_fixate_field_string(s, "stream-format", upstream_stream_format).
> That will chose the upstream stream format if possible, otherwise chose one of
> the possible ones.

Thank you for quick reply. I agree your point. The string field fixation is only done on the first structure since we already truncated the caps earlier. The purpose of this patch is, if downstream can support either stream-format, we should avoid fixating to a stream-format different from upstream. Suppose if h264parse get byte-stream/nal in input and downstream can support both byte-stream and avc but needs au alignment(avdec_h264) ... here we would end up fixating to avc/au (instead of the more efficient byte-stream/au). And also if the downstream element supports only avc and input to parser is byte-stream OR vice-versa ... then only conversion should be allowed. This patch exactly does this. I have verified this patch for possible cases. I couldn't find any issues. :)

This still produces an avc stream while I think it should be passthrough:

 gst-launch-1.0 -v filesrc location=test.h264 ! h264parse ! fakesink

-- 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/95.