Created attachment 364061 [details] [review]
h264parse: put downstream caps first if possible on sink caps

Try prioritizing downstream's caps over upstream's if possible so the
parser can configured in "passthrough" if possible and save it from
doing useless conversions.

Once approved I'll do a similar patch for h265parse.

Review of attachment 364061 [details] [review]:

Generally looks good

::: gst/videoparsers/gsth264parse.c
@@ +2787,2 @@
   if (peercaps) {
+    GstCaps *pcopy = gst_caps_copy (peercaps);

This claims that peercaps can be NULL but they can't, right? The above code always sets it to something, so if anything they might be empty but never NULL

@@ +2804,3 @@
 
+  /* Try if we can put the downstream caps first */
+  remove_fields (peercaps, FALSE);

... otherwise if they could be NULL, this would fail. So please remove the if condition above, or otherwise there will be a coverity warning about this :)

Right, gst_pad_peer_query_caps() will never return NULL. Fixing.

Created attachment 364102 [details] [review]
h264parse: put downstream caps first if possible on sink caps

Try prioritizing downstream's caps over upstream's if possible so the
parser can configured in "passthrough" if possible and save it from
doing useless conversions.

Created attachment 364104 [details] [review]
h265parse: put downstream caps first if possible on sink caps

Try prioritizing downstream's caps over upstream's if possible so the
parser can configured in "passthrough" if possible and save it from
doing useless conversions.

Exact same change as the one I just did in h264parse.

Attachment 364102 [details] pushed as d5067b4 - h264parse: put downstream caps first if possible on sink caps
Attachment 364104 [details] pushed as 0087485 - h265parse: put downstream caps first if possible on sink caps

Review of attachment 364102 [details] [review]:

::: tests/check/elements/h264parse.c
@@ +363,3 @@
+  GstElement *parser;
+  GstPad *sink, *src;
+  GstCaps *src_caps, *sink_caps;;

This double-semicolon did not compile with the autotools build system, fixed before pushing :)

The compiler warning flags are different between meson and autotools.

This causes the caps to be pushed twice, once without the interlace-mode and the second with it.

We can only know the interlace mode from the stream itself and not the codec_data? In that case this patch has to be reverted and there's nothing better we can do.

When I looked last, I was quite convinced that this information is in the codec data. But there is a lot of embedded if, and only one branch adding the interlace mode. So I suppose that we don't visit the right branch on first pass, and then run out of time. Guillaume as been sick pretty much all week and could not look at it yet.

For sure we should revert before 1.13.1 if not fixed on time.

Do you have an example input caps string and/or short gdp snippet that reproduces the problem?

For me caps are sent twice with any mov containing h264.

I notice because v4l2h264dec does not support renegotiation ATM, working on it. I also don't have code to differentiate meaningless caps change like this.

I can't reproduce this here with H264/MP4 and "filesrc ! qtdemux ! h264parse ! fakesink" or also "filesrc ! decodebin ! fakesink". The caps are only sent once on the srcpad of h264parse according to the gst-launch-1.0 -v output.

Is there still a problem here or did it get solved in the meantime?

I've been confusing with bug 790709 , I believe we can close this one ?

Review of attachment 364102 [details] [review]:

commit 7e45b9f03fdb89618eee2b3277173d94b547cc15
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Fri Mar 2 10:37:45 2018 -0500

    Revert "h264parse: early set src caps when input is avc"
    
    This reverts commit 5ac886d85aab4b919f84fb80e2d1ef36dc8e647d.

Review of attachment 364104 [details] [review]:

commit 9865904d8820c43a16d2d655ba31d155bb1ac581 (HEAD -> master)
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Fri Mar 2 10:37:53 2018 -0500

    Revert "h265parse: early set src caps when input not byte-stream"
    
    This reverts commit 93d29e80300f566b7a8e7d86beecb578fe03821c.

arg, sorry.