[webkit-reviews] review denied: [Bug 129774] [GStreamer] Disable video and/or audio if all tracks are deselected : [Attachment 334808] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied 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 334808: Patch

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




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

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

I think there are a couple of issues you would need to tackle that I am sorry
that I could not detect before, but the most important one is if there is a
test about this or you can create one.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
752
> +    GST_TRACE("Rendering %s frame", GST_IS_SAMPLE(m_sample.get()) ?
"valid":"black");

Nit: I think you would leave a space around :

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
756
> +	   if (UNLIKELY(!frameHolder->isValid()))
> +	       return;

I think we should either at least GST_WARNING here or default to black frame.
Maybe you can begin with a bool couldPaintFrame = false, set it to true just
after painting and paint the black frame if (!couldPaintFrame).

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
770
> +

Nit: I think you can remove this line.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
773
> +    bool validFrame = getSampleVideoInfo(m_sample.get(), videoInfo);

This variable should be called something like isFrameValid.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
879
> +    // FIXME: Flushing works correctly only for the gst-gl code

Please, add a reference to this bug.