Bug 106760 - [GStreamer] USE(NATIVE_FULLSCREEN_VIDEO) support
Summary: [GStreamer] USE(NATIVE_FULLSCREEN_VIDEO) support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 107398
  Show dependency treegraph
 
Reported: 2013-01-14 00:29 PST by Philippe Normand
Modified: 2013-01-31 03:47 PST (History)
14 users (show)

See Also:


Attachments
Patch (76.77 KB, patch)
2013-01-20 05:53 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (38.09 KB, patch)
2013-01-20 07:10 PST, Philippe Normand
gns: review-
Details | Formatted Diff | Diff
Patch (41.86 KB, patch)
2013-01-20 10:54 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (38.55 KB, patch)
2013-01-26 07:14 PST, Philippe Normand
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (42.27 KB, patch)
2013-01-26 07:37 PST, Philippe Normand
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (42.55 KB, patch)
2013-01-26 08:05 PST, Philippe Normand
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (46.80 KB, patch)
2013-01-26 08:36 PST, Philippe Normand
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (47.52 KB, patch)
2013-01-26 08:48 PST, Philippe Normand
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2013-01-14 00:29:57 PST
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.
Comment 1 Philippe Normand 2013-01-20 05:53:35 PST
Created attachment 183668 [details]
Patch
Comment 2 Philippe Normand 2013-01-20 07:10:57 PST
Created attachment 183671 [details]
Patch

Only WebCore changes. GTK changes will be part of another patch.
Comment 3 Gustavo Noronha (kov) 2013-01-20 09:41:14 PST
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?
Comment 4 Philippe Normand 2013-01-20 10:01:25 PST
(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.
Comment 5 Philippe Normand 2013-01-20 10:54:29 PST
Created attachment 183676 [details]
Patch
Comment 6 Philippe Normand 2013-01-20 10:56:50 PST
The PlatformVideoWindowGtk changes are indeed not needed. I think it was the result of early erroneous prototyping :)
Comment 7 Philippe Normand 2013-01-26 07:11:50 PST
(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.
Comment 8 Philippe Normand 2013-01-26 07:14:28 PST
Created attachment 184863 [details]
Patch
Comment 9 Early Warning System Bot 2013-01-26 07:24:59 PST
Comment on attachment 184863 [details]
Patch

Attachment 184863 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16113918
Comment 10 Early Warning System Bot 2013-01-26 07:26:31 PST
Comment on attachment 184863 [details]
Patch

Attachment 184863 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16116850
Comment 11 Philippe Normand 2013-01-26 07:37:35 PST
Created attachment 184866 [details]
Patch

Qt build fix attempt
Comment 12 Early Warning System Bot 2013-01-26 07:47:25 PST
Comment on attachment 184866 [details]
Patch

Attachment 184866 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16118846
Comment 13 Early Warning System Bot 2013-01-26 07:47:47 PST
Comment on attachment 184866 [details]
Patch

Attachment 184866 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16112947
Comment 14 Philippe Normand 2013-01-26 08:05:13 PST
Created attachment 184868 [details]
Patch
Comment 15 Early Warning System Bot 2013-01-26 08:13:18 PST
Comment on attachment 184868 [details]
Patch

Attachment 184868 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16112954
Comment 16 Early Warning System Bot 2013-01-26 08:13:40 PST
Comment on attachment 184868 [details]
Patch

Attachment 184868 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16119816
Comment 17 Philippe Normand 2013-01-26 08:36:51 PST
Created attachment 184869 [details]
Patch
Comment 18 Early Warning System Bot 2013-01-26 08:42:35 PST
Comment on attachment 184869 [details]
Patch

Attachment 184869 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16123812
Comment 19 Early Warning System Bot 2013-01-26 08:42:54 PST
Comment on attachment 184869 [details]
Patch

Attachment 184869 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16119830
Comment 20 Philippe Normand 2013-01-26 08:48:39 PST
Created attachment 184870 [details]
Patch
Comment 21 Gustavo Noronha (kov) 2013-01-28 05:04:23 PST
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.
Comment 22 Philippe Normand 2013-01-28 05:22:49 PST
(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!
Comment 23 Gustavo Noronha (kov) 2013-01-29 05:54:10 PST
(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?
Comment 24 Philippe Normand 2013-01-29 06:03:27 PST
This is what I understood yes,

if (volume > 1)
  volume = 1
else if (volume < 0)
  volume = 0
Comment 25 Philippe Normand 2013-01-30 06:58:19 PST
Committed r141265: <http://trac.webkit.org/changeset/141265>
Comment 26 Ádám Kallai 2013-01-31 02:17:43 PST
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.
Comment 27 Philippe Normand 2013-01-31 02:36:35 PST
(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 :)
Comment 28 Philippe Normand 2013-01-31 02:43:06 PST
(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
Comment 29 Philippe Normand 2013-01-31 02:50:20 PST
 p.type = PlatformMedia::None;

would be better I think. I'll test that fix here and land it.
Comment 30 Philippe Normand 2013-01-31 03:47:44 PST
http://trac.webkit.org/changeset/141398