Created attachment 364189 [details] [review]
h264parse: early set src caps when input is avc

When input is in AVC format there is no need to wait for the first buffer
before setting src caps. We already have all the information from the
input codec_data.

This allow us to already configure downstream elements allowing them,
for example, to already allocate their internal buffers as they know
the format of the input they are about to receive.

Comment on attachment 364189 [details] [review]
h264parse: early set src caps when input is avc

Same for h265parse I guess?

Created attachment 364212 [details] [review]
h265parse: early set src caps when input not byte-stream

When input is not in byte-stream format there is no need to wait for the first
buffer before setting src caps. We already have all the information from the
input codec_data.

This allow us to already configure downstream elements allowing them,
for example, to already allocate their internal buffers as they know
the format of the input they are about to receive.

Same change as the one I just did in h264parse.

Attachment 364189 [details] pushed as 5ac886d - h264parse: early set src caps when input is avc
Attachment 364212 [details] pushed as 93d29e8 - h265parse: early set src caps when input not byte-stream

These patches causes a regression, the caps are not pushed twice. Once without interlace-mode and then later on with the interlace-mode set. The fact the caps are different makes it a bit worst as it force a renegotiation. The pipeline I've used to test:

GST_DEBUG="*EVENT*:5" gst-launch-1.0 filesrc location=/home/nicolas/Vidéos/Tests/tintin-tlr2_h1080p.mov ! qtdemux ! h264parse ! video/x-h264,stream-format=byte-stream ! fakesink 2>&1 | grep "type caps"

You can also test with any of the .mov of BigBuck bunny that is H264 encoded.

This is still a problem from what I can see. Nicolas, Guillaume, is anybody of you working on this or should we revert the commits for 1.14?

Also this seems loosely related to https://bugzilla.gnome.org/show_bug.cgi?id=790628 but I can't reproduce that one.

I'm pretty busy atm so can't promise you I'll have time to look at it soon.
So yeah go ahead and revert, I'll resume this work as soon as I have some time.

I'll make sure I can still reproduce. Downstream caps could make a difference.

Could test on the DB410c, but I could not reproduce, and I tried with various downstream caps, no difference.

oops, "could not test on DB410c".

This one is reproducible with the pipeline in comment 5. I can't reproduce bug #790628 though

Review of attachment 364189 [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 364212 [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.

Reverted for the release, see comment #5 on how to reproduce the regressions.

I think the problem was we were ignoring the sink caps when early setting the src caps. If I fix that I'm no longer able to reproduce Nicolas's regression as we no longer loose fields like interlace-mode.

Here is the patch. If that looks ok to you I'll provide a similar one for h265parse.

Created attachment 369741 [details] [review]
h264parse: early set src caps when input is avc

When input is in AVC format there is no need to wait for the first buffer
before setting src caps. We already have all the information from the
input codec_data.

This allow us to already configure downstream elements allowing them,
for example, to already allocate their internal buffers as they know
the format of the input they are about to receive.

This patch is pretty similar to 5ac886d85aab4b919f84fb80e2d1ef36dc8e647d
which has been reverted but now we carry over fields from the sink caps.
By doing so we no longer end up with partial caps which were updated
later and so was triggering a renegotiation.

I've learn a lot since I first look at this patch, I'm not going to say this how it's implemented, but this is clearly a lie. I won't split anything in for 1.14 because this requires more work.

The case that is not handled here is if there is multiple SPS. If this is the case, you cannot do that, you have to wait for the frame, and look at which SPS is to be used in order to update the caps (if you don't want to cause a renegotiation). We just need code to catch this case, and also make sure we do check the SPS index in the frame header.

Isn't the demuxer going to use the first SPS as codec_data?
If it does then I think we're good as we already support SPS changes from I understand from the code.

No, all sps are together. It's the frames that specify which one becomes active. I'm not sure this works in practice, bits that how it's designed. This way you can have the decoder prepared, and switching can be faster.

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