Bug 153541 - [GTK] Implement mediastream mediaplayer
Summary: [GTK] Implement mediastream mediaplayer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alejandro G. Castro
URL:
Keywords:
Depends on: 153489
Blocks: 153826 153828
  Show dependency treegraph
 
Reported: 2016-01-27 03:48 PST by Alejandro G. Castro
Modified: 2016-02-04 03:42 PST (History)
2 users (show)

See Also:


Attachments
Patch (20.97 KB, patch)
2016-01-27 03:54 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (20.93 KB, patch)
2016-01-27 04:39 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (21.62 KB, patch)
2016-01-28 04:22 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (21.53 KB, patch)
2016-02-02 04:28 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (21.45 KB, patch)
2016-02-02 09:17 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (21.45 KB, patch)
2016-02-02 10:32 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (21.61 KB, patch)
2016-02-03 04:38 PST, Alejandro G. Castro
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2016-01-27 03:48:11 PST
We need to add the mediaplayer for the mediastream.
Comment 1 Alejandro G. Castro 2016-01-27 03:54:42 PST
Created attachment 269996 [details]
Patch
Comment 2 Philippe Normand 2016-01-27 04:04:06 PST
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 :)
Comment 3 Alejandro G. Castro 2016-01-27 04:38:51 PST
(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.
Comment 4 Alejandro G. Castro 2016-01-27 04:39:38 PST
Created attachment 269997 [details]
Patch
Comment 5 Alejandro G. Castro 2016-01-27 04:45:51 PST
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.
Comment 6 Philippe Normand 2016-01-27 05:56:17 PST
I suspect adding the new file in PlatformEFL.cmake would make the EWS a bit happier, maybe?
Comment 7 Martin Robinson 2016-01-27 06:39:46 PST
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.
Comment 8 Alejandro G. Castro 2016-01-28 04:00:15 PST
(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.
Comment 9 Alejandro G. Castro 2016-01-28 04:22:03 PST
Created attachment 270107 [details]
Patch
Comment 10 Eric Carlson 2016-01-28 07:05:24 PST
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?
Comment 11 Alejandro G. Castro 2016-01-28 07:20:43 PST
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.
Comment 12 Eric Carlson 2016-01-28 07:26:43 PST
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().
Comment 13 Alejandro G. Castro 2016-01-28 07:31:58 PST
(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.
Comment 14 Alejandro G. Castro 2016-02-02 04:28:36 PST
Created attachment 270484 [details]
Patch
Comment 15 Eric Carlson 2016-02-02 07:10:49 PST
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 };
Comment 16 Alejandro G. Castro 2016-02-02 09:16:36 PST
(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.
Comment 17 Alejandro G. Castro 2016-02-02 09:17:18 PST
Created attachment 270492 [details]
Patch
Comment 18 Alejandro G. Castro 2016-02-02 10:32:06 PST
Created attachment 270497 [details]
Patch
Comment 19 Martin Robinson 2016-02-02 16:50:09 PST
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
Comment 20 Alejandro G. Castro 2016-02-03 04:37:20 PST
Thanks for the comments Martin, I've updated all of them.
Comment 21 Alejandro G. Castro 2016-02-03 04:38:07 PST
Created attachment 270570 [details]
Patch
Comment 22 Zan Dobersek 2016-02-03 12:25:55 PST
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<...>(...);
Comment 23 Martin Robinson 2016-02-03 18:40:26 PST
Comment on attachment 270570 [details]
Patch

Okay. I think with Zan's nits this looks good. Thanks for the changes!
Comment 24 Philippe Normand 2016-02-04 00:56:52 PST
Looks good indeed, thanks for the reviews :)
Comment 25 Alejandro G. Castro 2016-02-04 02:47:36 PST
(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.
Comment 26 Alejandro G. Castro 2016-02-04 03:39:45 PST
Committed r196117: <http://trac.webkit.org/changeset/196117>
Comment 27 Alejandro G. Castro 2016-02-04 03:42:01 PST
Thanks everyone for the reviews.