[GStreamer][MediaStream] Handle track addition and removal
Created attachment 354698 [details] Patch
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().
Created attachment 354922 [details] Patch
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).
Created attachment 355036 [details] Patch
(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.
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.
Created attachment 355047 [details] Patch for landing
Created attachment 355048 [details] Patch for landing
> 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
Comment on attachment 355048 [details] Patch for landing Clearing flags on attachment: 355048 Committed r238276: <https://trac.webkit.org/changeset/238276>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46125460>