Bug 178076 – [MSE][GStreamer] Insert parser elements in AppendPipeline when demuxing opus or Vorbis

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alicia Boya García]

YouTube does not include durations in the webm container for files containing
Opus audio, so we need to read them from the contained stream. Fortunately,
GStreamer has an element to do that: opusparse.

[Alicia Boya García]

webm files from the W3C WPT containing Vorbis have the same problem.

Unfortunately, the output of vorbisparse is not as simple as opusparse's:
streamheader packets are emitted as GstBuffers downstream even when that same
initialization data has been already pushed with caps events.

Also, buffers with zero-duration appear. It's not yet clear whether this could
cause a problem.

[Alicia Boya García]

Created attachment 323400 [details]
Patch

[Enrique Ocaña]

Comment on attachment 323400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323400&action=review

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:955
> +    GRefPtr<GstCaps> padCaps = adoptGRef(gst_pad_get_current_caps(GST_PAD(demuxerSrcPad)));

Is this GST_PAD() macro actually needed?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:965
> +        return opusparse;

Just a comment for future reference without any action required on your side: opusparse is a floating reference. The return type is GRefPtr. This means that there's going to be a GRefPtr(opusparse) call under the hood which will call g_object_ref_sink() and convert the floating ref into a regular ref owned by the GRefPtr.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1034
> +        // This is known to be an issue with YouTube webm files containing Opus audio as of YT2018.

YouTube/tv (isn't the same as YouTube)

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:-1094
> -    // Must be done in the thread we were called from (usually streaming thread).

What happened to the black hole probe?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:-1111
> -        gst_element_unlink(m_demux.get(), m_decryptor.get());

No unlinking now? Why?

[Alicia Boya García]

Comment on attachment 323400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323400&action=review

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:955
>> +    GRefPtr<GstCaps> padCaps = adoptGRef(gst_pad_get_current_caps(GST_PAD(demuxerSrcPad)));
> 
> Is this GST_PAD() macro actually needed?

Not really, I should remove it.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:965
>> +        return opusparse;
> 
> Just a comment for future reference without any action required on your side: opusparse is a floating reference. The return type is GRefPtr. This means that there's going to be a GRefPtr(opusparse) call under the hood which will call g_object_ref_sink() and convert the floating ref into a regular ref owned by the GRefPtr.

That is intended in this case, though I could be more explicit, right.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1034
>> +        // This is known to be an issue with YouTube webm files containing Opus audio as of YT2018.
> 
> YouTube/tv (isn't the same as YouTube)

Regular YouTube also uses Opus, I would be surprised if the files were different in regard to durations in the container.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:-1094
>> -    // Must be done in the thread we were called from (usually streaming thread).
> 
> What happened to the black hole probe?

The condition in the `if` block was always false because by the time `pad-removed` is emitted the pad is already unlinked.
I'm not completely sure about probes lifecycle but I think they are removed automatically with the pad when the pad refcount hits zero (which should be soon after the `pad-removed` handler finishes unless it's re-linked there).

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:-1111
>> -        gst_element_unlink(m_demux.get(), m_decryptor.get());
> 
> No unlinking now? Why?

Removing the elements from the bin automatically unlinks them. It's easier to remove a series of optional elements than to remove links between a chain of optional elements then also removing the elements.

[Alicia Boya García]

Comment on attachment 323400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323400&action=review

>>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:-1094
>>> -    // Must be done in the thread we were called from (usually streaming thread).
>> 
>> What happened to the black hole probe?
> 
> The condition in the `if` block was always false because by the time `pad-removed` is emitted the pad is already unlinked.
> I'm not completely sure about probes lifecycle but I think they are removed automatically with the pad when the pad refcount hits zero (which should be soon after the `pad-removed` handler finishes unless it's re-linked there).

I just checked it by adding a dummy constructor that prints to a file every time it's invoked: the probes are being destroyed correctly.

[Alicia Boya García]

Created attachment 323657 [details]
Patch

[Enrique Ocaña]

Ok, then I don't have any more to comment. All your explanations sound reasonable. Just rebase the patch because it's not applying in trunk right now.

[Alicia Boya García]

Created attachment 323659 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 323659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323659&action=review

The nits are optional, at your discretion. Careful with the compilation.

> Source/WebCore/ChangeLog:6
> +        YouTube does not include durations in the webm container for files

WebM

> Source/WebCore/ChangeLog:10
> +        The same thing happens with Vorbis contained in webm files from the

Ditto.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:736
> +        GST_TRACE("append: trackId=%s PTS=%s DTS=%s DUR=%s presentationSize=%.0fx%.0f",
> +            mediaSample->trackID().string().utf8().data(),
> +            mediaSample->presentationTime().toString().utf8().data(),
> +            mediaSample->decodeTime().toString().utf8().data(),
> +            mediaSample->duration().toString().utf8().data(),
> +            mediaSample->presentationSize().width(), mediaSample->presentationSize().height());

Nit: You can have longer lines.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1034
> +        // This is known to be an issue with YouTube webm files containing Opus audio as of YT2018.

WebM and nit, I'd say YTTV2018.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1143
> +    GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS(GST_BIN(m_pipeline.get()), GST_DEBUG_GRAPH_SHOW_ALL, "pad-removed-before");

Nit: Why do we need so much pipeline dumping?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1148
> +    if (m_decrytor) {

This does not compile.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1161
> +    GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS(GST_BIN(m_pipeline.get()), GST_DEBUG_GRAPH_SHOW_ALL, "pad-removed-after");

Nit: Why do we need so much pipeline dumping?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:121
> +    GRefPtr<GstElement> m_parser; // optional

// Optional.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:241
> +    } else if (!g_strcmp0(mediaType, "video/x-vp8")
> +        || !g_strcmp0(mediaType, "video/x-vp9")
> +        || !g_strcmp0(mediaType, "audio/x-opus")
> +        || !g_strcmp0(mediaType, "audio/x-vorbis"))

Nit: You can have longer lines here.

> LayoutTests/ChangeLog:6
> +        YouTube does not include durations in the webm container for files

WebM

> LayoutTests/ChangeLog:10
> +        The same thing happens with Vorbis contained in webm files from the

Ditto.

[Alicia Boya García]

Comment on attachment 323659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323659&action=review

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:736
>> +            mediaSample->presentationSize().width(), mediaSample->presentationSize().height());
> 
> Nit: You can have longer lines.

Next time you are checking whether the number and order of parameters is OK you'll be grateful for them to be separate lines.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1161
>> +    GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS(GST_BIN(m_pipeline.get()), GST_DEBUG_GRAPH_SHOW_ALL, "pad-removed-after");
> 
> Nit: Why do we need so much pipeline dumping?

Should the unlinking fail or stop the player somehow we would like to know what happened in the append pipeline.

>> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:241
>> +        || !g_strcmp0(mediaType, "audio/x-vorbis"))
> 
> Nit: You can have longer lines here.

IMHO it's more readable this way: one line, one separate case where the condition is met.

[Alicia Boya García]

Created attachment 323899 [details]
Patch

[Alicia Boya García]

Created attachment 323901 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323901 [details]
Patch

Rejecting attachment 323901 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 323901, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/4880340

[Alicia Boya García]

Created attachment 324002 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 324002 [details]
Patch

Clearing flags on attachment: 324002

Committed r223505: <https://trac.webkit.org/changeset/223505>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.