[webkit-reviews] review granted: [Bug 129774] [GStreamer] Disable video and/or audio if all tracks are deselected : [Attachment 334743] 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 129774: [GStreamer] Disable video and/or audio if all tracks are deselected
https://bugs.webkit.org/show_bug.cgi?id=129774

Attachment 334743: Patch

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




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

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:308
> +void setGstPlayFlag(GstElement* playbin, const char* flagName, bool value)

I think here value is a bad name, I think we should use enabled.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:322
> +    GST_DEBUG("Setting %s flag to %s", flagName, value ?
"enabled":"disabled");

You can use boolForPrinting instead.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h:75
> +void setGstPlayFlag(GstElement*, const char* flagName, bool value);

Given the name of the function, you could make value->enabled default to true.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:752
> +    GST_INFO("%s %s track with index: %lu", enabled ?
"Enabling":"Disabling", flagName, index);

maybe boolForPrinting

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1894
> +		   GST_DEBUG("[Clock Changed] Restarting playback.");

Seeing dots at the end of GST log is kinda weird.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1900
> +	       if (m_fixingClock) {
> +		   GST_DEBUG("[Clock Changed] Restarting playback.");
> +		   m_fixingClock = false;
> +		   changePipelineState(GST_STATE_PLAYING);
> +	       } else if (didBuffering && !m_buffering && !m_paused &&
m_playbackRate) {
>		   GST_DEBUG("[Buffering] Restarting playback.");
>		   changePipelineState(GST_STATE_PLAYING);
>	       }

I would write this in a different way:
if (m_fixingClock || (didBuffering && !m_buffering && !m_paused &&
m_playbackRate)) {
    GST_DEBUG("%s Restarting playback", m_fixingClock ? "[Clock changed]" :
"[Buffering]");
    m_fixingClock = false;
    changePipelineState(GST_STATE_PLAYING);
}

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:140
> +    void setTrackEnabled(TrackType, unsigned index, bool enabled);

You can default enabled to true.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
749
> +    std::unique_ptr<TextureMapperPlatformLayerBuffer> layerBuffer;

layerBuffer is used only with GSTREAMER_GL, please move it inside it.a

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
787
> +	       texture->reset(size, BitmapTexture::NoFlag);

Maybe we should comment about painting a black frame here as well.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
891
> +	   WTF::GMutexLocker<GMutex> lock(m_sampleMutex);

Not for this patch, but we should use WTF::Lock and LockHolder here instead.

> Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.h:50
> +    void setTrackEnabled(bool);

For this kind of functions, I'm a big fan of the default to true :)