Bug 193715 – [WPE] Implement GStreamer based holepunch

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Miguel Gomez]

Add a holepunch feature that allows to perform video playback through GStreamer but, instead of drawing the frames, draws a transparent rectangle on the position where the video should be. The rendering of the frames is performed by a platform dependent video sink that puts the video on a layer below the browser, so it's visible through the transparent rectangle.

As the creation of the sink depends on the platform, the implementation uses a placeholder for that (a fakesink that doesn't draw anything to the page), and the function that sets the position and size of the video window is empty. Each platform should replace those pieces of code with the bits required.

[Miguel Gomez]

Created attachment 359874 [details]
Patch

[EWS Watchlist]

Comment on attachment 359874 [details]
Patch

Attachment 359874 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10857875

New failing tests:
imported/w3c/web-platform-tests/css/css-grid/abspos/orthogonal-positioned-grid-descendants-013.html

[EWS Watchlist]

Created attachment 359884 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Xabier Rodríguez Calvar]

Comment on attachment 359874 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:498
> +    return FloatSize(HOLEPUNCH_DEFAULT_FRAME_WIDTH, HOLEPUNCH_DEFAULT_FRAME_HEIGHT);

Considering that this is the only place where you use the default width and height, you could have a FloatSize s_holePunchDefaultSize, you could even make it static here and remove the defines from above.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1119
> +            LockHolder holder(proxy.lock());
> +            std::unique_ptr<TextureMapperPlatformLayerBuffer> layerBuffer = std::make_unique<TextureMapperPlatformLayerBuffer>(0, m_size, TextureMapperGL::ShouldNotBlend, GL_DONT_CARE);
> +            layerBuffer->setVideoSink(videoSink);
> +            proxy.pushNextBuffer(WTFMove(layerBuffer));

I see this lines repeated a lot. I think you could just create a new method in the proxy called pushNextEmptyBufferWithVideoSink (name to be improved) or if you thing the arch does not allow it, create a helper function.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:507
> +void TextureMapperGL::drawSolidColor(const FloatRect& rect, const TransformationMatrix& matrix, const Color& color, bool blend)

This parameter should be called shouldBlend (everywhere you use it)

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:42
> +static void setRectangleToVideoSink(GstElement* videoSink, IntRect rect)
> +{
> +    // Here goes the platform-dependant code to set to the videoSink the size
> +    // and position of the video rendering window. Mark them unused as default.
> +    UNUSED_PARAM(videoSink);
> +    UNUSED_PARAM(rect);

And here what is more concerning to me: this is introducing a new upstream video drawing path to be used downstream and that is completely untested. Believe me that I understand the need of reducing deltas ;) but this shouldn't go untested, to say the least.

[Philippe Normand]

Comment on attachment 359874 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1110
> +    GstElement* videoSink =  gst_element_factory_make("fakesink", nullptr);

Have you tried with fakevideosink instead?

[Miguel Gomez]

Created attachment 360455 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 360455 [details]
Patch

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

My biggest concern now is if we are breaking components encapsulation by introducing GStreamer stuff inside the texture mapper.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:497
> +    // is properly created.

is -> to be

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1099
> +GstElement* MediaPlayerPrivateGStreamerBase::createVideoSinkHolepunch()

createHolePunchVideoSink

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1103
> +    GstElement* videoSink =  gst_element_factory_make("fakesink", nullptr);

As Phil asked before, did you give fakevideosink a try?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:507
> +void TextureMapperGL::drawSolidColor(const FloatRect& rect, const TransformationMatrix& matrix, const Color& color, bool allowBlending)

allowBlending -> isBlendingAllowed

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:31
> +#include "FloatRect.h"

Is this necessary?

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:31
> +#include "GRefPtrGStreamer.h"

I think we'd want to move this below this section and flag it for HOLE_PUNCH

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:415
> +        if (m_nicosia.contentLayer)
> +            m_shouldUpdatePlatformLayer = true;

Is this supposed to be run always? It seems weird to see the rest of the code flagged by HOLE_PUNCH but not this.

> Source/cmake/OptionsWPE.cmake:50
> +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_HOLEPUNCH "Whether to enable GStreamer holepunch" PUBLIC OFF)

I don't think this option should be PUBLIC but PRIVATE instead.

[Miguel Gomez]

(In reply to Xabier Rodríguez Calvar from comment #7)
> Comment on attachment 360455 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360455&action=review
> 
> My biggest concern now is if we are breaking components encapsulation by
> introducing GStreamer stuff inside the texture mapper.

Yes. The thing is that animations run inside the compositor thread, at the TextureMapperLayer level, so that's the place where you know the real size and position for the player for each frame. And you need to set those values to the videoSink at that point, that's why it's taken there. I could pass it as a pointer instead of as a GstElement if we don't want to add GStreamer stuff there.
 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:497
> > +    // is properly created.
> 
> is -> to be

ok.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1099
> > +GstElement* MediaPlayerPrivateGStreamerBase::createVideoSinkHolepunch()
> 
> createHolePunchVideoSink
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1103
> > +    GstElement* videoSink =  gst_element_factory_make("fakesink", nullptr);
> 
> As Phil asked before, did you give fakevideosink a try?

Ah, forgot to change that.

> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:507
> > +void TextureMapperGL::drawSolidColor(const FloatRect& rect, const TransformationMatrix& matrix, const Color& color, bool allowBlending)
> 
> allowBlending -> isBlendingAllowed

ok

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:31
> > +#include "FloatRect.h"
> 
> Is this necessary?

Yes, as TextureMapperPlatformLayerBuffer::paintToTextureMapper() receives a FloatRect parameter that we're really using, instead of just passing it to another function.

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:31
> > +#include "GRefPtrGStreamer.h"
> 
> I think we'd want to move this below this section and flag it for HOLE_PUNCH

ok

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:415
> > +        if (m_nicosia.contentLayer)
> > +            m_shouldUpdatePlatformLayer = true;
> 
> Is this supposed to be run always? It seems weird to see the rest of the
> code flagged by HOLE_PUNCH but not this.

It's required for the holepunch case case we only push one buffer every now and then. For the other cases, as we are pushing a buffer for each frame, it's not required, but it doesn't hurt either.

> > Source/cmake/OptionsWPE.cmake:50
> > +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_HOLEPUNCH "Whether to enable GStreamer holepunch" PUBLIC OFF)
> 
> I don't think this option should be PUBLIC but PRIVATE instead.

From what I learned about the options, being public means that we should support both cases (enabled and disabled), while being private means that changing the default value may not be supported. In that sense I think it should be public IMO, unless there's some meaning that I'm missing.

[Miguel Gomez]

Created attachment 360459 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 360459 [details]
Patch

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

We're getting there...

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:98
> +#if USE(GSTREAMER_HOLEPUNCH)
> +    if (m_extraFlags & TextureMapperGL::ShouldNotBlend) {
> +        ASSERT(m_videoSink && !m_texture);
> +        texmapGL.drawSolidColor(targetRect, modelViewMatrix, Color(0, 0, 0, 0), false);
> +        setRectangleToVideoSink(m_videoSink.get(), enclosingIntRect(modelViewMatrix.mapRect(targetRect)));
> +        return;
> +    }
> +#endif

This is a bit of layer violation, we should probably create a client interface for this class that shoudl be implemented by the player and do this job there.

> Source/WebCore/rendering/RenderVideo.cpp:166
> +#if PLATFORM(WPE) && USE(GSTREAMER_HOLEPUNCH)
> +    // When using holepunch, skip all the intrinsic size calculations and just report the <video>
> +    // element size. The external application or the custom sink will be in charge of scaling the video
> +    // frames appropriately.
> +    return snappedIntRect(contentBoxRect());
> +#endif

I also feel we should abstract this behind the player.

> Source/cmake/OptionsWPE.cmake:50
> +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_HOLEPUNCH "Whether to enable GStreamer holepunch" PUBLIC OFF)

This is a functionality that can't work on its own, it needs something else (downstream code) to be complete, so I don't think it's a good idea to make this PUBLIC. It should be definitely PRIVATE, IMHO.

[Miguel Gomez]

Created attachment 361189 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 361189 [details]
Patch

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

Good, though there are some bits missing, we are getting there!

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

You can probably remove one of these two lines

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1108
> +class GSTHolepunchClient : public TextureMapperPlatformLayerBuffer::HolepunchClient {

GStreamerHolePunchClient

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1110
> +    GSTHolepunchClient(GstElement* videoSink) : m_videoSink(videoSink) { };

I would recommend receiving here GRefPtr<GstElement>&& and moving it into the attribute.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1114
> +    void setVideoRectangle(IntRect rect) override
> +    {
> +        setRectangleToVideoSink(m_videoSink.get(), rect);
> +    }

You can make this final and it is ok to put all in a single line.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1119
> +GstElement* MediaPlayerPrivateGStreamerBase::createHolepunchVideoSink()

createHolePunchVideoSink

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1128
> +void MediaPlayerPrivateGStreamerBase::pushNextHolepunchBuffer()

pushNextHolePunchBuffer

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1414
> +bool MediaPlayerPrivateGStreamerBase::shouldIgnoreIntrinsicSize()
> +{
> +#if USE(GSTREAMER_HOLEPUNCH)
> +    // When using holepunch, tell RenderVideo to ignore the intrinsicSize and just use the size defined through
> +    // css. This is because the platform videoSink will deal with the resize and positioning of the video frames
> +    // inside the assigned video rectangle.
> +    return true;
> +#endif
> +
> +    return false;
> +}
> +

Please, remove this because we can handle it in the .h

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:172
> +    bool shouldIgnoreIntrinsicSize() override;

Please, make this final, flag it with #GSTREAMER_HOLEPUNCH and return true. If the hole punch compile switch is not on, then the default false of MediaPlayerPrivate.h will be in place.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:180
> +    GstElement* createHolepunchVideoSink();
> +    void pushNextHolepunchBuffer();

As I said above Holepunch should be capitalized as HolePunch everywhere.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:82
> +#if USE(GSTREAMER_HOLEPUNCH)
> +    if (m_extraFlags & TextureMapperGL::ShouldNotBlend) {

My idea was to remove this compile switch from here and avoid involving GStreamer in this non media class. If you write it as if (m_holePunchClient && m_extraFlags... you can remove it.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:68
> +#if USE(GSTREAMER_HOLEPUNCH)

Ditto.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:69
> +    class HolepunchClient {

HolePunchClient

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:74
> +    void setHolepunchClient(std::unique_ptr<HolepunchClient> client) { m_holepunchClient = WTFMove(client); }

This should be std::unique_ptr<HolepunchClient>&& client to make the std::unique_ptr in the stack of the caller all the way down moved to here.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:89
> +#if USE(GSTREAMER_HOLEPUNCH)

Ditto

[Miguel Gomez]

Created attachment 361294 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 361294 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Implement GStreamer based holepunch functionality. Instead of getting the video frames from the
> +        video sink and drawing then, the player just draws a transparent rectangle on the position where
> +        the video should be. A platform dependent video sink will be used to render the video frames on
> +        a plane below the browser.

Please, explain the arch a bit more.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1558
> +bool MediaPlayer::shouldIgnoreIntrinsicSize()
> +{
> +    return m_private->shouldIgnoreIntrinsicSize();
> +}

Please, inline this in the .h

> Source/WebCore/platform/graphics/MediaPlayer.h:579
> +    bool shouldIgnoreIntrinsicSize();

As I said above, let's inline here.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:137
> +static FloatSize s_holepunchDefaultFrameSize(1280, 720);

static const FloatSize

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1099
> +static void setRectangleToVideoSink(GstElement* videoSink, IntRect& rect)

const IntRect&

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1131
> +            std::unique_ptr<GStreamerHolePunchClient> holepunchClient = std::make_unique<GStreamerHolePunchClient>(m_videoSink.get());

holePunchClient, please check if there are other appearances of this.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:70
> +        virtual void setVideoRectangle(IntRect) = 0;

Let optimize this by making it (const IntRect&) all the way down the onion.

[Miguel Gomez]

Agree on almost all the comments. Just a detail on this:
 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:1558
> > +bool MediaPlayer::shouldIgnoreIntrinsicSize()
> > +{
> > +    return m_private->shouldIgnoreIntrinsicSize();
> > +}
> 
> Please, inline this in the .h

There aren't inlined calls to the private inside MediaPlayer.h. MediaPlayerPrivateInterface is a forward declared class so we can't call methods from it unless we add the appropriate include, but I don't think that's desired in this case.

[Miguel Gomez]

Created attachment 361386 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to Miguel Gomez from comment #15)
> Agree on almost all the comments. Just a detail on this:
>  
> > > Source/WebCore/platform/graphics/MediaPlayer.cpp:1558
> > > +bool MediaPlayer::shouldIgnoreIntrinsicSize()
> > > +{
> > > +    return m_private->shouldIgnoreIntrinsicSize();
> > > +}
> > 
> > Please, inline this in the .h
> 
> There aren't inlined calls to the private inside MediaPlayer.h.
> MediaPlayerPrivateInterface is a forward declared class so we can't call
> methods from it unless we add the appropriate include, but I don't think
> that's desired in this case.

Oh, ok, let's forget this then.

[WebKit Commit Bot]

Comment on attachment 361386 [details]
Patch

Clearing flags on attachment: 361386

Committed r241120: <https://trac.webkit.org/changeset/241120>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.