[webkit-reviews] review granted: [Bug 178076] [MSE][GStreamer] Insert parser elements in AppendPipeline when demuxing opus or Vorbis : [Attachment 323659] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 16 03:59:53 PDT 2017
Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Alicia Boya García
<aboya at igalia.com>'s request for review:
Bug 178076: [MSE][GStreamer] Insert parser elements in AppendPipeline when
demuxing opus or Vorbis
https://bugs.webkit.org/show_bug.cgi?id=178076
Attachment 323659: Patch
https://bugs.webkit.org/attachment.cgi?id=323659&action=review
--- Comment #9 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 323659
--> https://bugs.webkit.org/attachment.cgi?id=323659
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.
More information about the webkit-reviews
mailing list