Bug 162899 – [GStreamer][MSE] MediaSourceGStreamer refactoring

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

[Enrique Ocaña]

Move MediaSourceGStreamer to the mse directory, manage SourceBufferPrivates and delegate WebKitMediaSrc operations to the MSE private player.

[Enrique Ocaña]

Created attachment 290591 [details]
Patch

[Enrique Ocaña]

Comment on attachment 290591 [details]
Patch

Wait until all the patches in 157314 are ready.

[Xabier Rodríguez Calvar]

Comment on attachment 290591 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:49
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/glib/GRefPtr.h>

These two lines are not alphabetically sorted.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:71
> +    for (HashSet<SourceBufferPrivateGStreamer*>::iterator iterator = m_sourceBuffers.begin(), end = m_sourceBuffers.end(); iterator != end; ++iterator)
> +        (*iterator)->clearMediaSource();

I am not sure if this would work, but you might want to try C++11 features like:
for (sourceBuffer : m_sourceBuffers)
    sourceBuffer->clearMediaSource();

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:76
> +    RefPtr<SourceBufferPrivateGStreamer> buffer = SourceBufferPrivateGStreamer::create(this, m_client, contentType);

Please, name this variable properly. This is not a buffer.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.h:71
> +    virtual AddStatus addSourceBuffer(const ContentType&, RefPtr<SourceBufferPrivate>&);
> +    void removeSourceBuffer(SourceBufferPrivate*);
> +
> +    virtual void durationChanged();
> +    virtual void markEndOfStream(EndOfStreamStatus);
> +    virtual void unmarkEndOfStream();
> +
> +    virtual MediaPlayer::ReadyState readyState() const;
> +    virtual void setReadyState(MediaPlayer::ReadyState);
> +
> +    virtual void waitForSeekCompleted();
> +    virtual void seekCompleted();

These functions should not be tagged as virtual but as override.

[Xabier Rodríguez Calvar]

Comment on attachment 290591 [details]
Patch

I would like to have a deeper look to pointers here as I suspect we could use more refcounted

[Enrique Ocaña]

(In reply to comment #4)

> I would like to have a deeper look to pointers here as I suspect we could
> use more refcounted

I've converted m_sourceBuffers to be a HashSet<RefPtr<SourceBufferPrivateGStreamer>>, and m_mediaSource is now a RefPtr<MediaSourcePrivateClient>. I'll be submitting an updated patch soon.

For the case of m_activeSourceBuffers and m_playerPrivate I think it's fine to use raw pointers. In the first case, there won't be any active source buffer which is not in already in the set of source buffers. This is alse the strategy followed by Apple's MediaPlayerPrivateMediaSourceAVFObjC.

For the case of m_playerPrivate, the thing is that none of MediaPlayerPrivateGStreamerMSE or its ancestors inherit from RefCounted. MediaPlayerPrivateInterface is just not meant to be RefCounted, because MediaPlayer holds an std::unique_ptr<MediaPlayerPrivateInterface>. Therefore, all the reference holding elsewhere must be done using raw pointers.

[Enrique Ocaña]

Created attachment 291762 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 291762 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.h:82
> +    RefPtr<MediaSourcePrivateClient> m_mediaSource;

Are we sure we don't have a circular reference here?

[Enrique Ocaña]

(In reply to comment #7)

> > Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.h:82
> > +    RefPtr<MediaSourcePrivateClient> m_mediaSource;
> Are we sure we don't have a circular reference here?

Yes, I've checked that the destructors of both MediaSourcePrivateClient and MediaSourceGStreamer are called at runtime. It wouldn't be so if we had a circular reference.

[Enrique Ocaña]

Created attachment 292254 [details]
Patch

[Zan Dobersek]

Comment on attachment 292254 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:56
> +    ASSERT(mediaSource);
> +    ASSERT(playerPrivate);

Use references for the parameters if you're expecting non-null values, not pointers.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:58
> +    RefPtr<MediaSourceGStreamer> mediaSourcePrivate = adoptRef(new MediaSourceGStreamer(*mediaSource, *playerPrivate));

Use a Ref<> and adoptRef(*new ...) combo.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:59
> +    mediaSourcePrivate->m_playerPrivate = playerPrivate;

Do this in the constructor.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:60
> +    mediaSource->setPrivateAndOpen(mediaSourcePrivate.releaseNonNull());

You'd move mediaSourcePrivate (now a Ref<>) in here, but since the m_playerPrivate assignment would be moved into the constructor, you might as well inline the adoptRef(*new MediaSourceGStreamer) call into this setPrivateAndOpen() call. So together with using references for parameter types of this open() method, you should end up with a one-liner.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:67
> +    m_client = MediaSourceClientGStreamerMSE::create(playerPrivate);

m_client can be assigned in the initialization list of this constructor.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:72
> +    for (RefPtr<SourceBufferPrivateGStreamer> sourceBufferPrivate : m_sourceBuffers)

for (auto& sourceBufferPrivate : m_sourceBuffers)

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.cpp:86
> +    RefPtr<SourceBufferPrivateGStreamer> sourceBufferPrivateGStreamer = reinterpret_cast<SourceBufferPrivateGStreamer*>(sourceBufferPrivate);

This should be a static_cast, like in the method above.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.h:82
> +    RefPtr<MediaSourcePrivateClient> m_mediaSource;

This can be a Ref<>.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceGStreamer.h:83
> +    MediaPlayerPrivateGStreamerMSE* m_playerPrivate;

This can be a reference.

[Enrique Ocaña]

Created attachment 292408 [details]
Patch

[Zan Dobersek]

Comment on attachment 292408 [details]
Patch

LGTM.

[Enrique Ocaña]

Created attachment 292893 [details]
Patch

[Enrique Ocaña]

Comment on attachment 292893 [details]
Patch

Clearing flags on attachment: 292893

Committed r207880: <http://trac.webkit.org/changeset/207880>

[Enrique Ocaña]

All reviewed patches have been landed.  Closing bug.