Bug 191599 – [GStreamer][MediaStream] Handle track addition and removal

• Component: New Bugs
• Status: RESOLVED FIXED

[Thibault Saunier]

[GStreamer][MediaStream] Handle track addition and removal

[Thibault Saunier]

Created attachment 354698 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 354698 [details]
Patch

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

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:154
> +    void characteristicsChanged() final { GST_DEBUG_OBJECT(m_mediaStreamSrc.get(), "renegotiation should happen."); }

Nit: is it common to have periods as GStreamer logging messages?

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:488
> +static void
> +webkitMediaStreamSrcAddTrack(WebKitMediaStreamSrc* self, MediaStreamTrackPrivate* track)

same line

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:498
> +    if (track->type() == RealtimeMediaSource::Type::Audio) {
> +        webkitMediaStreamSrcSetupAppSrc(self, track, &self->audioSrc,
> +            &audioSrcTemplate);
> +    } else if (track->type() == RealtimeMediaSource::Type::Video) {
> +        webkitMediaStreamSrcSetupAppSrc(self, track, &self->videoSrc,
> +            &videoSrcTemplate);
> +    } else {
> +        GST_INFO("Unsuported track type: %d", (gint) track->type());
>      }

For such small lines you don't need to split in two. Besides, brackets should go away as they are one line clauses.

UnsuPPorted.

(gint) -> static_cast<int>(...)

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:502
> +static void
> +webkitMediaStreamSrcRemoveTrackByType(WebKitMediaStreamSrc* self, RealtimeMediaSource::Type trackType)

same line

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:510
> +

No need for this line

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:519
> +    } else {
> +        GST_INFO("Unsuported track type: %d", (gint) trackType);
>      }

No need for brackets and fix the cast, please.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:522
> +gboolean webkitMediaStreamSrcSetStream(WebKitMediaStreamSrc* self, MediaStreamPrivate* stream)

This function should return bool

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:524
> +    g_return_val_if_fail(WEBKIT_IS_MEDIA_STREAM_SRC(self), FALSE);

I don't think this is needed here, right?

Anyway, if you want to keep such a test it should be ASSERT().

[Thibault Saunier]

Created attachment 354922 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 354922 [details]
Patch

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

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:494
> +        GST_INFO("Unsupported track type: %d", (gint) track->type());

Please, use static_cast here.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:505
>  

This line does not make sense here. Please remove it.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:513
> +        GST_INFO("Unsupported track type: %d", (gint) trackType);

static_cast, please

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:518
> +    g_return_val_if_fail(WEBKIT_IS_MEDIA_STREAM_SRC(self), FALSE);

I don't think this is needed here. If you still consider it needed, please turn it into an ASSERT instead of g_return_val_if_fail (which btw, should be returning false instead of FALSE now that it is a bool instead of a gboolean).

[Thibault Saunier]

Created attachment 355036 [details]
Patch

[Thibault Saunier]

(In reply to Xabier Rodríguez Calvar from comment #4)
> Comment on attachment 354922 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354922&action=review
> 
> > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:494
> > +        GST_INFO("Unsupported track type: %d", (gint) track->type());
> 
> Please, use static_cast here.

Done.

> > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:505
> >  
> 
> This line does not make sense here. Please remove it.
> 
> > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:513
> > +        GST_INFO("Unsupported track type: %d", (gint) trackType);
> 
> static_cast, please

Done

> > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:518
> > +    g_return_val_if_fail(WEBKIT_IS_MEDIA_STREAM_SRC(self), FALSE);
> 
> I don't think this is needed here. If you still consider it needed, please
> turn it into an ASSERT instead of g_return_val_if_fail (which btw, should be
> returning false instead of FALSE now that it is a bool instead of a
> gboolean).


Well it is external API, better safe guard it?
ASSERT is not the same thing, `g_return_val_if_fail` leads to a runtime warning, not an ASSERT by default.

[Xabier Rodríguez Calvar]

Comment on attachment 355036 [details]
Patch

(In reply to Thibault Saunier from comment #6)
> > > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:518
> > > +    g_return_val_if_fail(WEBKIT_IS_MEDIA_STREAM_SRC(self), FALSE);
> > 
> > I don't think this is needed here. If you still consider it needed, please
> > turn it into an ASSERT instead of g_return_val_if_fail (which btw, should be
> > returning false instead of FALSE now that it is a bool instead of a
> > gboolean).
> 
> Well it is external API, better safe guard it?
> ASSERT is not the same thing, `g_return_val_if_fail` leads to a runtime
> warning, not an ASSERT by default.

If it remains inside WebKit without being exposed to C or GTK APIs, then we should stick to the ASSERT.

[Thibault Saunier]

Created attachment 355047 [details]
Patch for landing

[Thibault Saunier]

Created attachment 355048 [details]
Patch for landing

[Thibault Saunier]

> If it remains inside WebKit without being exposed to C or GTK APIs, then we should stick to the ASSERT.

You are right, just id that. Sorry for the noise

[WebKit Commit Bot]

Comment on attachment 355048 [details]
Patch for landing

Clearing flags on attachment: 355048

Committed r238276: <https://trac.webkit.org/changeset/238276>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46125460>