Bug 153541 – [GTK] Implement mediastream mediaplayer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Alejandro G. Castro]

We need to add the mediaplayer for the mediastream.

[Alejandro G. Castro]

Created attachment 269996 [details]
Patch

[Philippe Normand]

Comment on attachment 269996 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:201
> +            g_object_set(m_videoRenderer, "width", 640, "height", 480, nullptr);

Please add a FIXME about this :)

[Alejandro G. Castro]

(In reply to comment #2)
> Comment on attachment 269996 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269996&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:201
> > +            g_object_set(m_videoRenderer, "width", 640, "height", 480, nullptr);
> 
> Please add a FIXME about this :)

Fixed, thanks.

[Alejandro G. Castro]

Created attachment 269997 [details]
Patch

[Alejandro G. Castro]

The failure in efl bot is caused by a problem that should be solved when patch in bug 153489 lands, we can wait for that to check again.

[Philippe Normand]

I suspect adding the new file in PlatformEFL.cmake would make the EWS a bit happier, maybe?

[Martin Robinson]

Comment on attachment 269997 [details]
Patch

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

Not a GStreamer person, but I noticed a few issues with the patch.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:98
> +    GstQuery* query= gst_query_new_position(GST_FORMAT_TIME);

Nit "query=" -> "query ="

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:105
> +    float result = 0.0f;

This should just be float result = 0;

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:128
> +    if (!m_streamPrivate || !m_streamPrivate->active()) {

!m_streamPrivate can never be true right? Perhaps this check should be omitted entirely.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:142
> +    // HaveEnoughData

Missing a period here.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:152
> +

Extra line.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:166
> +{
> +    return true;
> +}

Is this implementation complete or does it need a FIXME as well?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:243
> +    // FIXME

Do you mind actually writing a fixme comment here that is a complete sentence?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:248
> +    // FIXME

Ditto.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:252
> +bool MediaPlayerPrivateGStreamerOwr::isAvailable()

This should probably be renamed to something like initializeGStreamerAndGStreamerDebugging.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:262
> +    static bool debugRegistered = false;
> +    if (!debugRegistered) {
> +        GST_DEBUG_CATEGORY_INIT(webkit_openwebrtc_debug, "webkitowrplayer", 0, "WebKit OpenWebRTC player");
> +        debugRegistered = true;
> +    }
> +

I think you can use std::once here.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:277
> +    m_audioRenderer = owr_gst_audio_renderer_new(m_audioSink.get());

It looks like this is leaking?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:317
> +    m_videoRenderer = owr_gst_video_renderer_new(sink);

It looks like this is leaking as well. Better to use smart pointers, if possible.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:318
> +    return nullptr;

Why do you always return nullptr? Certainly this cannot be correct.

[Alejandro G. Castro]

(In reply to comment #7)
> Comment on attachment 269997 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269997&action=review
> 
> Not a GStreamer person, but I noticed a few issues with the patch.
> 

Awesome review Martin, thanks very much. I basically did everything and I'm going to upload again.

[Alejandro G. Castro]

Created attachment 270107 [details]
Patch

[Eric Carlson]

Comment on attachment 270107 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        * html/HTMLMediaElement.cpp:
> +        (WebCore::HTMLMediaElement::setSrcObject):
> +        (WebCore::HTMLMediaElement::createMediaPlayer): Deleted.
> +        * platform/graphics/MediaPlayer.cpp:
> +        (WebCore::buildMediaEnginesVector):
> +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp: Added.

I think it is helpful to have comments in the ChangeLog explaining what changed or was added. This isn't as necessary with completely new files, but please at least add comments about the changes to existing files.

Also, "(WebCore::HTMLMediaElement::createMediaPlayer): Deleted" suggests that the entire method was deleted, which is not true.

> Source/WebCore/html/HTMLMediaElement.cpp:-5769
> -#if ENABLE(MEDIA_STREAM)
> -    m_mediaStreamSrcObject = nullptr;
> -#endif
> -

Why is this unnecessary?

[Alejandro G. Castro]

Thanks for the comments.

(In reply to comment #10)
> Comment on attachment 270107 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270107&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        * html/HTMLMediaElement.cpp:
> > +        (WebCore::HTMLMediaElement::setSrcObject):
> > +        (WebCore::HTMLMediaElement::createMediaPlayer): Deleted.
> > +        * platform/graphics/MediaPlayer.cpp:
> > +        (WebCore::buildMediaEnginesVector):
> > +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp: Added.
> 
> I think it is helpful to have comments in the ChangeLog explaining what
> changed or was added. This isn't as necessary with completely new files, but
> please at least add comments about the changes to existing files.
> 
> Also, "(WebCore::HTMLMediaElement::createMediaPlayer): Deleted" suggests
> that the entire method was deleted, which is not true.
> 

I'll do it.

> > Source/WebCore/html/HTMLMediaElement.cpp:-5769
> > -#if ENABLE(MEDIA_STREAM)
> > -    m_mediaStreamSrcObject = nullptr;
> > -#endif
> > -
> 
> Why is this unnecessary?

It is already nullified in the constructor.

Thanks again for the comment.

[Eric Carlson]

Comment on attachment 270107 [details]
Patch

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

>>> Source/WebCore/html/HTMLMediaElement.cpp:-5769
>>> -
>> 
>> Why is this unnecessary?
> 
> It is already nullified in the constructor.
> 
> Thanks again for the comment.

createMediaPlayer is called at the beginning of the load algorithm, so may be called multiple times during the life cycle of a media element (eg. if a script changes .src or calls load().

[Alejandro G. Castro]

(In reply to comment #12)
> Comment on attachment 270107 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270107&action=review
> 
> >>> Source/WebCore/html/HTMLMediaElement.cpp:-5769
> >>> -
> >> 
> >> Why is this unnecessary?
> > 
> > It is already nullified in the constructor.
> > 
> > Thanks again for the comment.
> 
> createMediaPlayer is called at the beginning of the load algorithm, so may
> be called multiple times during the life cycle of a media element (eg. if a
> script changes .src or calls load().

Good point, I agree, I'll check the code again, thanks.

[Alejandro G. Castro]

Created attachment 270484 [details]
Patch

[Eric Carlson]

Comment on attachment 270484 [details]
Patch

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

I know almost nothing about GStreamer, so once the structural issues noted here have been fixed a GStreamer person should do the final review.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:47
> +    , m_paused(true)
> +    , m_stopped(true)

In new code we should initialize data members in the class definition.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.h:35
> +class URL;

This is not needed.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.h:52
> +    virtual String engineDescription() const { return "OpenWebRTC"; }
> +
> +    virtual void load(const String&);
> +#if ENABLE(MEDIA_SOURCE)
> +    virtual void load(const String&, MediaSourcePrivateClient*) { }
> +#endif
> +    virtual void load(MediaStreamPrivate&);
> +    virtual void cancelLoad() { }
> +
> +    virtual void prepareToPlay() { }

These are overrides, so you should omit the "virtual" and add "override", eg.:

    void load(const String&) override;

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.h:57
> +    void play();
> +    void pause();
> +
> +    bool hasVideo() const;
> +    bool hasAudio() const;

Aren't these supposed to override the base class?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.h:73
> +    virtual float duration() const { return 0; }
> +
> +    virtual float currentTime() const;
> +    virtual void seek(float) { }
> +    virtual bool seeking() const { return false; }
> +
> +    virtual void setRate(float) { }
> +    virtual void setPreservesPitch(bool) { }
> +    virtual bool paused() const { return m_paused; }
> +
> +    virtual bool hasClosedCaptions() const { return false; }
> +    virtual void setClosedCaptionsVisible(bool) { };
> +
> +    virtual float maxTimeSeekable() const { return 0; }
> +    virtual std::unique_ptr<PlatformTimeRanges> buffered() const { return std::make_unique<PlatformTimeRanges>(); }

Omit "virtual" and add "override"

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.h:74
> +    bool didLoadingProgress() const;

Aren't these supposed to override the base class?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.h:82
> +    virtual unsigned long long totalBytes() const { return 0; }
> +    virtual unsigned bytesLoaded() const { return 0; }
> +
> +    virtual bool canLoadPoster() const { return false; }
> +    virtual void setPoster(const String&) { }
> +
> +    virtual bool isLiveStream() const { return true; }

Omit "virtual" and add "override"

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.h:88
> +    virtual void sourceStopped() override final;
> +    virtual void sourceMutedChanged() override final;
> +    virtual void sourceSettingsChanged() override final;
> +    virtual bool preventSourceFromStopping() override final;

Don't need "virtual".

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.h:91
> +    virtual GstElement* createVideoSink();

Omit "virtual" and add "override"

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.h:93
> +private:

Almost everything in the file (eg. most or all of the virtual methods) can be private.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.h:105
> +    bool m_paused;
> +    bool m_stopped;

These should be initialized here instead of in the constructor:

    bool m_paused { false };
    bool m_stopped { false };

[Alejandro G. Castro]

(In reply to comment #15)
> Comment on attachment 270484 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270484&action=review
> 
> I know almost nothing about GStreamer, so once the structural issues noted
> here have been fixed a GStreamer person should do the final review.
> 

Thanks again for the review Eric, just did the changes and solved other small issues I found.

[Alejandro G. Castro]

Created attachment 270492 [details]
Patch

[Alejandro G. Castro]

Created attachment 270497 [details]
Patch

[Martin Robinson]

Comment on attachment 270497 [details]
Patch

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

I'm not a GStreamer expert, but I think this really close! I noticed a couple weird things though:

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:48
> +        LOG_MEDIA_MESSAGE("Creating Stream media player");

This is a little bit ambiguous. Perhaps say something like "Creating MediaPlayerPrivateGStreamerOwr."

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:146
> +    m_player->readyStateChanged();
> +
> +}

Nit: Extra line here.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:188
> +        if (track->type() == RealtimeMediaSource::Audio) {

It might be clearer to use a switch statement here and handle all enum values.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:200
> +            // FIXME: Remove hardcoded video dimensions when the rendering performance
> +            // bottleneck is resolved.

Would you file a bug for this and include a link here?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:206
> +        } else
> +            ASSERT_NOT_REACHED();

What does it mean when the track is None here? Might be worth leaving a comment explaining why it should never happen.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:266
> +    // FIXME: volume/mute support.

Would be a good to have a bug for this as well.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:283
> +    if (!m_streamPrivate || !m_streamPrivate->active())
> +        stop();
> +
> +    for (auto track : m_streamPrivate->tracks()) {

This doesn't seem right. You check if m_steamPrivate is NULL and then you immediately dereference it. If m_streamPrivate can be null it should be handled, but if not, it should be an assertion instead of a runtime check

[Alejandro G. Castro]

Thanks for the comments Martin, I've updated all of them.

[Alejandro G. Castro]

Created attachment 270570 [details]
Patch

[Zan Dobersek]

Comment on attachment 270497 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:25
> +#include "config.h"
> +
> +#if ENABLE(MEDIA_STREAM) && USE(GSTREAMER) && USE(OPENWEBRTC)
> +#include "MediaPlayerPrivateGStreamerOwr.h"

Nit: since normally the header that's specific to some implementation file uses the same build guards, I couple the config.h and this implementation-specific header into the same block, same as it's done when there are no build guards used.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:284
> +    for (auto track : m_streamPrivate->tracks()) {
> +        RealtimeMediaSourceOwr* source = reinterpret_cast<RealtimeMediaSourceOwr*>(&track->source());

You should use auto&, directly referencing the objects in the vector instead of copying the RefPtr object.

By specifying the type in reinterpret_cast, you can again simply use auto:

    auto* source = reinterpret_cast<...>(...);

[Martin Robinson]

Comment on attachment 270570 [details]
Patch

Okay. I think with Zan's nits this looks good. Thanks for the changes!

[Philippe Normand]

Looks good indeed, thanks for the reviews :)

[Alejandro G. Castro]

(In reply to comment #22)
> Comment on attachment 270497 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270497&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:25
> > +#include "config.h"
> > +
> > +#if ENABLE(MEDIA_STREAM) && USE(GSTREAMER) && USE(OPENWEBRTC)
> > +#include "MediaPlayerPrivateGStreamerOwr.h"
> 
> Nit: since normally the header that's specific to some implementation file
> uses the same build guards, I couple the config.h and this
> implementation-specific header into the same block, same as it's done when
> there are no build guards used.
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:284
> > +    for (auto track : m_streamPrivate->tracks()) {
> > +        RealtimeMediaSourceOwr* source = reinterpret_cast<RealtimeMediaSourceOwr*>(&track->source());
> 
> You should use auto&, directly referencing the objects in the vector instead
> of copying the RefPtr object.
> 
> By specifying the type in reinterpret_cast, you can again simply use auto:
> 
>     auto* source = reinterpret_cast<...>(...);

Thanks for the comments Zan, I'll apply them.

[Alejandro G. Castro]

Committed r196117: <http://trac.webkit.org/changeset/196117>

[Alejandro G. Castro]

Thanks everyone for the reviews.