[webkit-reviews] review denied: [Bug 117039] [GStreamer] Support audio and video tracks : [Attachment 210176] Update patch based on changes to text track patch

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has denied Brendan Long
<b.long at cablelabs.com>'s request for review:
Bug 117039: [GStreamer] Support audio and video tracks
https://bugs.webkit.org/show_bug.cgi?id=117039

Attachment 210176: Update patch based on changes to text track patch
https://bugs.webkit.org/attachment.cgi?id=210176&action=review

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=210176&action=review


> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.h:36
> +class AudioTrackPrivateGStreamer : public AudioTrackPrivate {

I suppose this class can be FINAL

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.h:59
> +#ifdef GST_API_VERSION_1

This is checked already, can be removed.

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.h:73
> +#ifdef GST_API_VERSION_1

Ditto

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:640
> +	   GstPad* pad;

Would it be possible to use a GRefPtr here?

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:695
> +	   GstPad* pad;

Ditto?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:192
> +    Vector<RefPtr<AudioTrackPrivateGStreamer> > m_audioTracks;

The space between > can be removed now.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:194
> +    Vector<RefPtr<VideoTrackPrivateGStreamer> > m_videoTracks;

Ditto

> Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.cpp:73
> +    g_signal_connect(m_pad.get(), "notify::tags",
G_CALLBACK(videoTrackPrivateTagsChangedCallback), this);

No need for a pad probe like in AudioTrack here? Also I wonder, can some parts
of both classes be factored out to a common class?

> Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.h:36
> +class VideoTrackPrivateGStreamer : public VideoTrackPrivate {

FINAL ?