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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=178076

Xabier Rodríguez Calvar <calvaris at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |calvaris at igalia.com
 Attachment #323659|review?                     |review+, commit-queue-
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171016/37a1bd8c/attachment.html>