[webkit-reviews] review granted: [Bug 210840] [GStreamer][MediaStream] fast/mediastream/play-newly-added-audio-track.html is failing since added in r260380 : [Attachment 426203] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 210840: [GStreamer][MediaStream]
fast/mediastream/play-newly-added-audio-track.html is failing since added in
r260380
https://bugs.webkit.org/show_bug.cgi?id=210840

Attachment 426203: Patch

https://bugs.webkit.org/attachment.cgi?id=426203&action=review




--- Comment #4 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 426203
  --> https://bugs.webkit.org/attachment.cgi?id=426203
Patch

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

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1660
> +	  
webkitMediaStreamSrcConfigureAudioTracks(WEBKIT_MEDIA_STREAM_SRC(m_source.get()
), volume(), isMuted(), !m_isPaused);

Can't we _CAST here?

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:12
5
> +    InternalSource(GstElement* parent, MediaStreamTrackPrivate& track,
String padName)

You can take a const String& here and let assignment to the attribute do the
copy.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:48
2
> +	   GST_OBJECT_UNLOCK(self);
> +	   return;
> +    }
> +
> +    auto upstreamId = priv->stream ? priv->stream->id() :
createCanonicalUUIDString();
> +    priv->streamCollection =
adoptGRef(gst_stream_collection_new(upstreamId.ascii().data()));
> +    for (auto& track : priv->tracks) {
> +	   if (!track->isActive())
> +	       continue;
> +	   gst_stream_collection_add_stream(priv->streamCollection.get(),
webkitMediaStreamNew(track.get()));
> +    }
> +
> +    GST_OBJECT_UNLOCK(self);

For these cases I would like to use ScopeExit on Scope.h. That way you could
create the ScopeExit to run the GST_OBJECT_UNLOCK and forget to UNLOCK in the
first place. In case you need to add more early returns, you're covered.

The problem is that ScopeExit does not have a runEarlyAndRelease method yet
that you would need for the second UNLOCK. If you feel like adding it, you have
my blessing. The patter would be similar to the unlockEarly of the LockHolder.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:52
3
> +    const String& padName;

ProbeData is most probably not going to outlive the InternalSource but it would
be safer to not have const String& and just a String here. We just make a copy
of the String object, the StringImpl does the refcounting and everybody is
happier.