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.
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.
Created attachment 323400 [details] Patch
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?
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.
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.
Created attachment 323657 [details] Patch
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.
Created attachment 323659 [details] Patch
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.
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.
Created attachment 323899 [details] Patch
Created attachment 323901 [details] Patch
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
Created attachment 324002 [details] Patch
Comment on attachment 324002 [details] Patch Clearing flags on attachment: 324002 Committed r223505: <https://trac.webkit.org/changeset/223505>
All reviewed patches have been landed. Closing bug.