Bug 106760 – [GStreamer] USE(NATIVE_FULLSCREEN_VIDEO) support

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

Until we have proper video AC support we could port the GStreamerGWorld module to 1.0 and reuse it under the above-mentioned guards. This code path would also be useful for devices that have no GL support but good autovideosink support, like the OLPC.

I started a patch for this issue, will try to clean it up and upload this week for review.

[Philippe Normand]

Created attachment 183668 [details]
Patch

[Philippe Normand]

Created attachment 183671 [details]
Patch

Only WebCore changes. GTK changes will be part of another patch.

[Gustavo Noronha (kov)]

Comment on attachment 183671 [details]
Patch

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

LGTM in general, I'll r- just because I'm worried about the checks in PlatformVideoWindowGtk might be hiding a bug, might be better to figure it out

> Source/WebCore/ChangeLog:20
> +        * platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.cpp: Added.

Are you going to move the FullscreenVideoController from WebKit/gtk/WebCoreSupport into WebCore to reuse it for WebKit2 and so on?

> Source/WebCore/platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.cpp:29
> +

No newline

> Source/WebCore/platform/graphics/gstreamer/GStreamerGWorld.cpp:237
> +    // Unlink and release request pad.
> +    gst_pad_unlink(srcPad.get(), sinkPad.get());
> +    gst_element_release_request_pad(tee.get(), srcPad.get());
> +
> +    // Unlink, remove and cleanup queue, ffmpegcolorspace, videoScale and sink.

I know they were there before, but these comments do not seem to add anything, what do you think of removing them?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1760
> +#if USE(NATIVE_FULLSCREEN_VIDEO)
> +void MediaPlayerPrivateGStreamer::enterFullscreen()
> +{
> +    // TODO.
> +}
> +
> +void MediaPlayerPrivateGStreamer::exitFullscreen()
> +{
> +    // TODO.
> +}

We should use notImplemented() here

> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowGtk.cpp:54
>          gtk_container_remove(GTK_CONTAINER(m_window), m_videoWindow);
> -        gtk_widget_destroy(m_videoWindow);
> +        if (GTK_IS_WIDGET(m_videoWindow))
> +            gtk_widget_destroy(m_videoWindow);

Is this protecting against m_videoWindow having been destroyed by being removed from the container? It looks a bit hacky

> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowGtk.cpp:58
> -    if (m_window) {
> +    if (GTK_IS_WIDGET(m_window)) {

Same here, the way I understand it m_window has to be a GtkWidget, am I misunderstanding?

[Philippe Normand]

(In reply to comment #3)
> (From update of attachment 183671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183671&action=review
> 
> LGTM in general, I'll r- just because I'm worried about the checks in PlatformVideoWindowGtk might be hiding a bug, might be better to figure it out
> 

Thanks for the review :)

> > Source/WebCore/ChangeLog:20
> > +        * platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.cpp: Added.
> 
> Are you going to move the FullscreenVideoController from WebKit/gtk/WebCoreSupport into WebCore to reuse it for WebKit2 and so on?
> 

Yes! See also bug 107398. I haven't yet updated the WK2 code but it should be easy.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1760
> > +#if USE(NATIVE_FULLSCREEN_VIDEO)
> > +void MediaPlayerPrivateGStreamer::enterFullscreen()
> > +{
> > +    // TODO.
> > +}
> > +
> > +void MediaPlayerPrivateGStreamer::exitFullscreen()
> > +{
> > +    // TODO.
> > +}
> 
> We should use notImplemented() here
> 

Well the bug above fixes those already.

> > Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowGtk.cpp:54
> >          gtk_container_remove(GTK_CONTAINER(m_window), m_videoWindow);
> > -        gtk_widget_destroy(m_videoWindow);
> > +        if (GTK_IS_WIDGET(m_videoWindow))
> > +            gtk_widget_destroy(m_videoWindow);
> 
> Is this protecting against m_videoWindow having been destroyed by being removed from the container? It looks a bit hacky
> 

Hacky indeed, I was getting some warnings when destroying a non-GtkWidget IIRC. I'll check those again.

> > Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowGtk.cpp:58
> > -    if (m_window) {
> > +    if (GTK_IS_WIDGET(m_window)) {
> 
> Same here, the way I understand it m_window has to be a GtkWidget, am I misunderstanding?

No, something bad is going on indeed.

[Philippe Normand]

Created attachment 183676 [details]
Patch

[Philippe Normand]

The PlatformVideoWindowGtk changes are indeed not needed. I think it was the result of early erroneous prototyping :)

[Philippe Normand]

(In reply to comment #6)
> The PlatformVideoWindowGtk changes are indeed not needed. I think it was the result of early erroneous prototyping :)

Well actually... the doc of gtk_container_remove mentions that it will unref the removed widget and thus destroy it as well. That doc also suggests that it's more simple and efficient to directly gtk_widget_destroy the widget.

[Philippe Normand]

Created attachment 184863 [details]
Patch

[Early Warning System Bot]

Comment on attachment 184863 [details]
Patch

Attachment 184863 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16113918

[Early Warning System Bot]

Comment on attachment 184863 [details]
Patch

Attachment 184863 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16116850

[Philippe Normand]

Created attachment 184866 [details]
Patch

Qt build fix attempt

[Early Warning System Bot]

Comment on attachment 184866 [details]
Patch

Attachment 184866 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16118846

[Early Warning System Bot]

Comment on attachment 184866 [details]
Patch

Attachment 184866 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16112947

[Philippe Normand]

Created attachment 184868 [details]
Patch

[Early Warning System Bot]

Comment on attachment 184868 [details]
Patch

Attachment 184868 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16112954

[Early Warning System Bot]

Comment on attachment 184868 [details]
Patch

Attachment 184868 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16119816

[Philippe Normand]

Created attachment 184869 [details]
Patch

[Early Warning System Bot]

Comment on attachment 184869 [details]
Patch

Attachment 184869 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16123812

[Early Warning System Bot]

Comment on attachment 184869 [details]
Patch

Attachment 184869 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16119830

[Philippe Normand]

Created attachment 184870 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 184870 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.cpp:125
> +{
> +    if (volume < 0.0 || volume > 1.0)
> +        return;
> +
> +    m_player->setVolume(volume);
> +}

Looks like volume will always be set using 0.05 increments, is that right? Wondering if it wouldn't make sense to volume = 1.0 if volume > 1.0, and set it, so that if the volume was at 0.98, say, and it got increaseVolume, it would go to 1.0. And for volume < 0 as well.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1764
> +    // TODO.

I know you said this is fixed in another bug already, but I'd prefer if this had notImplemented() while that patch doesn't land.

[Philippe Normand]

(In reply to comment #21)
> (From update of attachment 184870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184870&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.cpp:125
> > +{
> > +    if (volume < 0.0 || volume > 1.0)
> > +        return;
> > +
> > +    m_player->setVolume(volume);
> > +}
> 
> Looks like volume will always be set using 0.05 increments, is that right? Wondering if it wouldn't make sense to volume = 1.0 if volume > 1.0, and set it, so that if the volume was at 0.98, say, and it got increaseVolume, it would go to 1.0. And for volume < 0 as well.
> 

So basically you'd like to have the volume really clamped between 0 and 1? I think that's doable :)

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1764
> > +    // TODO.
> 
> I know you said this is fixed in another bug already, but I'd prefer if this had notImplemented() while that patch doesn't land.

Alright :) Thanks for the review!

[Gustavo Noronha (kov)]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 184870 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=184870&action=review
> > 
> > > Source/WebCore/platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.cpp:125
> > > +{
> > > +    if (volume < 0.0 || volume > 1.0)
> > > +        return;
> > > +
> > > +    m_player->setVolume(volume);
> > > +}
> > 
> > Looks like volume will always be set using 0.05 increments, is that right? Wondering if it wouldn't make sense to volume = 1.0 if volume > 1.0, and set it, so that if the volume was at 0.98, say, and it got increaseVolume, it would go to 1.0. And for volume < 0 as well.
> > 
> 
> So basically you'd like to have the volume really clamped between 0 and 1? I think that's doable :)
 
It already is clamped, what I'm wondering about is, given volume is increased and decreased in 0.05 increments/decrements, if you for some reason have the volume at 0.98, increasing the volume would take it to 1.03, and that if check would fail, so the volume would be kept at 0.98, but what you wanted was that it went to 1.0, makes sense?

[Philippe Normand]

This is what I understood yes,

if (volume > 1)
  volume = 1
else if (volume < 0)
  volume = 0

[Philippe Normand]

Committed r141265: <http://trac.webkit.org/changeset/141265>

[Ádám Kallai]

The Qt build is broken.
http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/47558/steps/compile-webkit/logs/stdio

Could you check it please?
If NATIVE_FULLSCREEN_VIDEO is disabled than a uninitialized variable will be returned. It only fails on this very bot - it passes on all of the other bots for some misterious reason.

[Philippe Normand]

(In reply to comment #26)
> The Qt build is broken.
> http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/47558/steps/compile-webkit/logs/stdio
> 
> Could you check it please?
> If NATIVE_FULLSCREEN_VIDEO is disabled than a uninitialized variable will be returned. It only fails on this very bot - it passes on all of the other bots for some misterious reason.

Right you need to add

#else
UNUSED_PARAM(p)

in ::platformMedia(). And make sure to include <wtf/UnusedParam.h>.

rs=me for that build fix :)

[Philippe Normand]

(In reply to comment #27)
> (In reply to comment #26)
> > The Qt build is broken.
> > http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/47558/steps/compile-webkit/logs/stdio
> > 
> > Could you check it please?
> > If NATIVE_FULLSCREEN_VIDEO is disabled than a uninitialized variable will be returned. It only fails on this very bot - it passes on all of the other bots for some misterious reason.
> 
> Right you need to add
> 
> #else
> UNUSED_PARAM(p)
> 
> in ::platformMedia(). And make sure to include <wtf/UnusedParam.h>.
> 
> rs=me for that build fix :)

Hum sorry this might not be the right fix

[Philippe Normand]

 p.type = PlatformMedia::None;

would be better I think. I'll test that fix here and land it.

[Philippe Normand]

http://trac.webkit.org/changeset/141398