[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