Bug 162896 – [GStreamer][MSE] MediaSourceClientGStreamerMSE

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Enrique Ocaña]

Encapsulate the MediaSource interaction responsibility into a class.

[Enrique Ocaña]

Created attachment 290589 [details]
Patch

[Enrique Ocaña]

Created attachment 290593 [details]
Patch

[Enrique Ocaña]

Comment on attachment 290593 [details]
Patch

Wait until all the patches in 157314 are ready.

[Xabier Rodríguez Calvar]

Comment on attachment 290593 [details]
Patch

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

These things can be fixed at landing time

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:47
> +    m_playerPrivate = playerPrivate;

Can't you do this assigment in the constructor?

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:63
> +    GST_DEBUG("this=%p sourceBuffer=%p appendPipeline=%p", this, sourceBufferPrivate.get(), appendPipeline.get());

You might want to add a more meaningful message and probably GST_TRACE instead.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:66
> +    ASSERT(m_playerPrivate->m_playbackPipeline);

If we are going to assert this, let's do it sooner, after the first return.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:141
> +    ASSERT(m_playerPrivate->m_playbackPipeline);

Let's assert this sooner, after the first return.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:177
> +        return GRefPtr<WebKitMediaSrc>(nullptr);
> +
> +    WebKitMediaSrc* source = WEBKIT_MEDIA_SRC(m_playerPrivate->m_source.get());
> +
> +    ASSERT(WEBKIT_IS_MEDIA_SRC(source));
> +
> +    return GRefPtr<WebKitMediaSrc>(source);

Let's do the implicit constructor do its job if possible.

[Enrique Ocaña]

Created attachment 291760 [details]
Patch

[Zan Dobersek]

Comment on attachment 291760 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:32
> +PassRefPtr<MediaSourceClientGStreamerMSE> MediaSourceClientGStreamerMSE::create(MediaPlayerPrivateGStreamerMSE* playerPrivate)

This should return Ref<>. And probably take playerPrivate via a reference, assuming it is not null.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:42
> +MediaSourceClientGStreamerMSE::MediaSourceClientGStreamerMSE(MediaPlayerPrivateGStreamerMSE* playerPrivate)

Ditto on playerPrivate being a reference.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:70
> +MediaTime MediaSourceClientGStreamerMSE::duration()

This method can return const MediaTime&.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:90
> +void MediaSourceClientGStreamerMSE::abort(PassRefPtr<SourceBufferPrivateGStreamer> prpSourceBufferPrivate)

This shouldn't be receiving a PassRefPtr<>. Supposing the argument is not null, it should be a Ref<>.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:99
> +    RefPtr<SourceBufferPrivateGStreamer> protectedPrpSourceBufferPrivate = prpSourceBufferPrivate;

With a Ref<> or even a RefPtr<> argument, you now don't need this protector.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:100
> +    RefPtr<AppendPipeline> appendPipeline = m_playerPrivate->m_appendPipelinesMap.get(protectedPrpSourceBufferPrivate);

There should be an assert that ensures the SourceBufferPrivateGStreamer object is in the map.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:104
> +bool MediaSourceClientGStreamerMSE::append(PassRefPtr<SourceBufferPrivateGStreamer> prpSourceBufferPrivate, const unsigned char* data, unsigned length)

Ditto on the PassRefPtr argument.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:113
> +    RefPtr<SourceBufferPrivateGStreamer> protectedPrpSourceBufferPrivate = prpSourceBufferPrivate;

Ditto on not needing a protector anymore.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:118
> +    return GST_FLOW_OK == appendPipeline->pushNewBuffer(buffer);

Comparing a return value to a constant is easier to read.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:125
> +    m_playerPrivate->markEndOfStream(status);

This method doesn't check for a null m_playerPrivate.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:137
> +    RefPtr<AppendPipeline> appendPipeline = m_playerPrivate->m_appendPipelinesMap.get(sourceBufferPrivate);

An assert would be welcome to make sure a pointer to the object is present in the map.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:152
> +    if (!m_playerPrivate)
> +        return;
> +
> +    m_playerPrivate->m_playbackPipeline->flushAndEnqueueNonDisplayingSamples(samples);

These null-checked single calls are better just compressed into two lines, without an early return:

    if (m_playerPrivate)
        m_playerPrivate->whatever().

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.h:36
> +class ContentType;
> +class SourceBufferPrivateGStreamer;
> +class MediaPlayerPrivateGStreamerMSE;
> +class MediaSample;

Alphabetic order.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.h:38
> +class MediaSourceClientGStreamerMSE: public RefCounted<MediaSourceClientGStreamerMSE> {

A missing space before the colon.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.h:64
> +    // Would better be a RefPtr, but the playerprivate is a unique_ptr, so we can't mess with references here. In
> +    // exchange, the playerprivate must notify us when it's being destroyed, so we can clear our pointer.

This comment can be removed, it's not used in other patches.

[Zan Dobersek]

Comment on attachment 291760 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:28
> +#if ENABLE(VIDEO) && USE(GSTREAMER) && ENABLE(MEDIA_SOURCE) && ENABLE(VIDEO_TRACK)

Why do you use ENABLE(VIDEO_TRACK) in general? This is meant to be used to guard HTML5 video subtitles support. I don't think it has anything to do with MSE.

[Enrique Ocaña]

(In reply to comment #7)

> Why do you use ENABLE(VIDEO_TRACK) in general? This is meant to be used to
> guard HTML5 video subtitles support. I don't think it has anything to do
> with MSE.

The ENABLE(VIDEO_TRACK) is used to guard code which needs "track" support (whatever is related to TrackPrivateBase). This includes audio, video and text tracks (subtitles).

There's code (eg: the regular MediaPlayerPrivateGStreamer) which in theory can work with track support disabled. MSE, on the contrary, is strongly coupled with the track support[1], so it makes no sense to enable one without the other. That's why all the code related to MSE is also guarded with ENABLE(VIDEO_TRACK).

[1] https://w3c.github.io/media-source/#video-track-extensions

[Enrique Ocaña]

Created attachment 292252 [details]
Patch

[Zan Dobersek]

(In reply to comment #8)
> (In reply to comment #7)
> 
> > Why do you use ENABLE(VIDEO_TRACK) in general? This is meant to be used to
> > guard HTML5 video subtitles support. I don't think it has anything to do
> > with MSE.
> 
> The ENABLE(VIDEO_TRACK) is used to guard code which needs "track" support
> (whatever is related to TrackPrivateBase). This includes audio, video and
> text tracks (subtitles).
> 
> There's code (eg: the regular MediaPlayerPrivateGStreamer) which in theory
> can work with track support disabled. MSE, on the contrary, is strongly
> coupled with the track support[1], so it makes no sense to enable one
> without the other. That's why all the code related to MSE is also guarded
> with ENABLE(VIDEO_TRACK).
> 
> [1] https://w3c.github.io/media-source/#video-track-extensions

Here's what's guarded with VIDEO_TRACK in Modules/mediasource/:
- three attributes on the SourceBuffer interface,
- extensions to the AudioTrack, TextTrack and VideoTrack interfaces.

So if a build is configured with MEDIA_SOURCE enabled and VIDEO_TRACK disabled, those attributes and interface extensions would disappear, leaving the rest of MSE APIs exposed, but the whole underlying GStreamer MSE supprot would vanish.

[Enrique Ocaña]

(In reply to comment #10)

> Here's what's guarded with VIDEO_TRACK in Modules/mediasource/:
> - three attributes on the SourceBuffer interface,
> - extensions to the AudioTrack, TextTrack and VideoTrack interfaces.
>
> So if a build is configured with MEDIA_SOURCE enabled and VIDEO_TRACK
> disabled, those attributes and interface extensions would disappear, leaving
> the rest of MSE APIs exposed, but the whole underlying GStreamer MSE supprot
> would vanish.

No. If a build is configured with MEDIA_SOURCE enabled and VIDEO_TRACK disabled, it will fail. To start with, because the methods guarded in SourceBuffer.h aren't guarded in SourceBuffer.cpp. SourceBuffer::m_{audio,video,text}Tracks are used all over the place unguarded.

Also, SourceBufferPrivateClient (SourceBuffer's superclass) manages the concept of InitializationSegment, which holds {Audio,Video,Text}TrackInformation with references to {Audio,Video,Text}TrackPrivate. Everything unguarded.

Therefore, my conclusion is that MEDIA_SOURCE and VIDEO_TRACK are indivisibly related. That's why I ask for both in all our private GStreamer implementation.

Does it really make sense to "fix the world" just to have ENABLE(VIDEO_TRACK) nicely splitted in the GStreamer private implementation?

[Zan Dobersek]

(In reply to comment #11)
> (In reply to comment #10)
> 
> > Here's what's guarded with VIDEO_TRACK in Modules/mediasource/:
> > - three attributes on the SourceBuffer interface,
> > - extensions to the AudioTrack, TextTrack and VideoTrack interfaces.
> >
> > So if a build is configured with MEDIA_SOURCE enabled and VIDEO_TRACK
> > disabled, those attributes and interface extensions would disappear, leaving
> > the rest of MSE APIs exposed, but the whole underlying GStreamer MSE supprot
> > would vanish.
> 
> No. If a build is configured with MEDIA_SOURCE enabled and VIDEO_TRACK
> disabled, it will fail. To start with, because the methods guarded in
> SourceBuffer.h aren't guarded in SourceBuffer.cpp.
> SourceBuffer::m_{audio,video,text}Tracks are used all over the place
> unguarded.
> 
> Also, SourceBufferPrivateClient (SourceBuffer's superclass) manages the
> concept of InitializationSegment, which holds
> {Audio,Video,Text}TrackInformation with references to
> {Audio,Video,Text}TrackPrivate. Everything unguarded.
> 
> Therefore, my conclusion is that MEDIA_SOURCE and VIDEO_TRACK are
> indivisibly related. That's why I ask for both in all our private GStreamer
> implementation.
> 
> Does it really make sense to "fix the world" just to have
> ENABLE(VIDEO_TRACK) nicely splitted in the GStreamer private implementation?

'Indivisibly related'? 'Fix the world'? It's a matter of a few patches to fix this.

OptionsGTK.cmake enables VIDEO_TRACK option but keeps it private. IMO that should be a good enough guarantee that it's always enabled as long as ENABLE_VIDEO is as well. MEDIA_SOURCE will depend on ENABLE_VIDEO as well.

VIDEO_TRACK should probably be taken as enabled by default, as long as video is supported.

[Zan Dobersek]

(In reply to comment #12)
> 'Indivisibly related'? 'Fix the world'? It's a matter of a few patches to
> fix this.
> 

To clarify -- it's trivially fixable, but don't bother with it. There's no one disabling VIDEO_TRACK but keeping MEDIA_SOURCE enabled, obviously. Just assume that if VIDEO is enabled, so is VIDEO_TRACK, and MEDIA_SOURCE relies on VIDEO, hence you don't have to support a configuration where VIDEO_TRACK is disabled but MEDIA_SOURCE is enabled. That's why you shouldn't bother with VIDEO_TRACK guards in platform/graphics/gstreamer/mse/.

[Enrique Ocaña]

Created attachment 292406 [details]
Patch

[Build Bot]

Comment on attachment 292406 [details]
Patch

Attachment 292406 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2340538

New failing tests:
svg/wicd/test-rightsizing-b.xhtml

[Build Bot]

Created attachment 292419 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Enrique Ocaña]

Created attachment 292779 [details]
Patch

[Enrique Ocaña]

Created attachment 292891 [details]
Patch

[Enrique Ocaña]

Comment on attachment 292891 [details]
Patch

Clearing flags on attachment: 292891

Committed r207878: <http://trac.webkit.org/changeset/207878>

[Enrique Ocaña]

All reviewed patches have been landed.  Closing bug.