Bug 42234 – Implementing VideoLayerChromium for gpu acceleration

• Component: New Bugs
• Status: RESOLVED FIXED

[Victoria Kirst]

Implementing VideoLayerChromium for gpu acceleration

[Victoria Kirst]

Created attachment 61472 [details]
Patch

[David Levin]

Seems fine.

<Very optional>
imo, it would be nice to include a few more header files in places where things are used (like RefPtr, PassRefPtr, or adoptRef being used without including the corresponding header file (as this seems to be using the fact that some other header file happens to include it ), but this isn't critical (and I may even be recommending something that other WK reviewers would recommend against).
</Very optional>


I'd like someone else from the video team to give an LGTM before I do the final r+. (If you want this to be committed automatically for you, please mark this as cq? in addition to r?.)

[Victoria Kirst]

Created attachment 61534 [details]
Patch

[Victoria Kirst]

Comment on attachment 61534 [details]
Patch

Deleted a method that VideoLayerChromium was overriding since it just called the super class's method with the same argument.

[Hin-Chung Lam]

Comment on attachment 61534 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 572b5565d11552d8037e1448eb9625d8f6b5fc59..49981a59a5f396a50fdf29defde7788b19ec364f 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,25 @@
> [phone]  Victoria Kirst  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Added a simple implementation of VideoLayerChromium. Uses the
> +        LayerChromium::updateTextureRect() to send video frames to the
> +        GPU.
> +        https://bugs.webkit.org/show_bug.cgi?id=42234
> +
> +        * WebCore.gypi: Added include for VideoLayerChromium.
> +        * platform/graphics/chromium/GraphicsLayerChromium.cpp:
> +        (WebCore::GraphicsLayerChromium::setContentsToMedia): Implemented
> +        setContentsToMedia, though it does not seem to trigger a repaint
> +        correctly.
> +        * platform/graphics/chromium/GraphicsLayerChromium.h:
> +        * platform/graphics/chromium/VideoLayerChromium.cpp: Added.
> +        (WebCore::VideoLayerChromium::create):
> +        (WebCore::VideoLayerChromium::VideoLayerChromium):
> +        (WebCore::VideoLayerChromium::updateTextureContents):
> +        * platform/graphics/chromium/VideoLayerChromium.h: Added.
> +        (WebCore::VideoLayerChromium::drawsContent):
> +
>  2010-07-07  Kristian Monsen  <[e-mail]>
>  
>          Reviewed by Steve Block.
> diff --git a/WebCore/WebCore.gypi b/WebCore/WebCore.gypi
> index 4edc42683aa27040ab44a4e3aad2df371bd2bfe7..74fd1ad731c64f21562d94e3981efe0383086cb1 100644
> --- a/WebCore/WebCore.gypi
> +++ b/WebCore/WebCore.gypi
> @@ -2194,6 +2194,8 @@
>              'platform/graphics/chromium/UniscribeHelper.h',
>              'platform/graphics/chromium/UniscribeHelperTextRun.cpp',
>              'platform/graphics/chromium/UniscribeHelperTextRun.h',
> +            'platform/graphics/chromium/VideoLayerChromium.cpp',
> +            'platform/graphics/chromium/VideoLayerChromium.h',
>              'platform/graphics/chromium/WebGLLayerChromium.cpp',
>              'platform/graphics/chromium/WebGLLayerChromium.h',
>              'platform/graphics/filters/FEBlend.cpp',
> diff --git a/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp b/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp
> index 9b65346835a6e8a215d4f8048ecf4a70ac6f0a29..a01a17fd2a5984f682a908ebae5b870685c0717e 100644
> --- a/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp
> @@ -363,9 +363,30 @@ void GraphicsLayerChromium::setContentsToWebGL(PlatformLayer* platformLayer)
>  }
>  #endif
>  
> -void GraphicsLayerChromium::setContentsToVideo(PlatformLayer* videoLayer)
> +void GraphicsLayerChromium::setContentsToMedia(PlatformLayer* layer)
>  {
> -    // FIXME: Implement
> +    bool childrenChanged = false;
> +    if (layer) {
> +        if (!m_contentsLayer.get() || m_contentsLayerPurpose != ContentsLayerForVideo) {
> +            setupContentsLayer(layer);
> +            m_contentsLayer = layer;
> +            m_contentsLayerPurpose = ContentsLayerForVideo;
> +            childrenChanged = true;
> +        }
> +        layer->setOwner(this);
> +        layer->setNeedsDisplay();
> +        updateContentsRect();
> +    } else {
> +        if (m_contentsLayer) {
> +            childrenChanged = true;
> +  
> +            // The old contents layer will be removed via updateSublayerList.
> +            m_contentsLayer = 0;
> +        }
> +    }
> +  
> +    if (childrenChanged)
> +        updateSublayerList();
>  }
>  
>  void GraphicsLayerChromium::setGeometryOrientation(CompositingCoordinatesOrientation orientation)
> diff --git a/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h b/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h
> index ccd02eb3f5ddf9820a50b979776359cb002c39d7..cd5e4799ab4f031a41831200e8a58d27920e3420 100644
> --- a/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h
> +++ b/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h
> @@ -85,7 +85,7 @@ public:
>      virtual void setContentsRect(const IntRect&);
>  
>      virtual void setContentsToImage(Image*);
> -    virtual void setContentsToVideo(PlatformLayer*);
> +    virtual void setContentsToMedia(PlatformLayer*);
>      virtual void setContentsToWebGL(PlatformLayer*);
>  
>      virtual PlatformLayer* platformLayer() const;
> diff --git a/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp b/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp
> new file mode 100644
> index 0000000000000000000000000000000000000000..5ac0e578dfe57c5f2c105194fb4ab1e92404a713
> --- /dev/null
> +++ b/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp
> @@ -0,0 +1,50 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "config.h"
> +
> +#if USE(ACCELERATED_COMPOSITING)
> +
> +#include "VideoLayerChromium.h"
> +
> +namespace WebCore {
> +
> +PassRefPtr<VideoLayerChromium> VideoLayerChromium::create(GraphicsLayerChromium* owner)
> +{
> +    return adoptRef(new VideoLayerChromium(owner));
> +}
> +
> +VideoLayerChromium::VideoLayerChromium(GraphicsLayerChromium* owner)
> +    : LayerChromium(owner)
> +{
> +}
> +
> +}
> +#endif // USE(ACCELERATED_COMPOSITING)
> diff --git a/WebCore/platform/graphics/chromium/VideoLayerChromium.h b/WebCore/platform/graphics/chromium/VideoLayerChromium.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..1fa80095dc865c3836b2ab326ed4b3fa4953014a
> --- /dev/null
> +++ b/WebCore/platform/graphics/chromium/VideoLayerChromium.h
> @@ -0,0 +1,54 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +
> +#ifndef VideoLayerChromium_h
> +#define VideoLayerChromium_h
> +
> +#if USE(ACCELERATED_COMPOSITING)
> +
> +#include "LayerChromium.h"
> +
> +namespace WebCore {
> +
> +// A Layer that contains a Video element.
> +class VideoLayerChromium : public LayerChromium {
> +public:
> +    static PassRefPtr<VideoLayerChromium> create(GraphicsLayerChromium* owner = 0);
> +    virtual bool drawsContent() { return true; }
> +
> +private:
> +    VideoLayerChromium(GraphicsLayerChromium* owner);
> +};
> +
> +}
> +#endif // USE(ACCELERATED_COMPOSITING)
> +
> +#endif
> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index 1598848de90af968534a7a6daac910e122712cdc..22489da44a1d43ecd5567ed98557e03301914d5d 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,17 @@
> [phone]  Victoria Kirst  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Updated WebMediaPlayer to support accelerated rendering and to
> +        create and return a VideoChromiumLayer as its platform layer.
> +        https://bugs.webkit.org/show_bug.cgi?id=42234
> +
> +        * src/WebMediaPlayerClientImpl.cpp:
> +        (WebKit::WebMediaPlayerClientImpl::platformLayer):
> +        (WebKit::WebMediaPlayerClientImpl::create):
> +        * src/WebMediaPlayerClientImpl.h:
> +        (WebKit::WebMediaPlayerClientImpl::supportsAcceleratedRendering):
> +
>  2010-07-07  Sheriff Bot  <[e-mail]>
>  
>          Unreviewed, rolling out r62645.
> diff --git a/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp b/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp
> index 03051c32bf3e2583773dd38fb009523bd2405d18..b6aa961bc0f0c58f864dfaac147dd3f1f41f69f1 100644
> --- a/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp
> +++ b/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp
> @@ -15,6 +15,7 @@
>  #include "MediaPlayer.h"
>  #include "NotImplemented.h"
>  #include "TimeRanges.h"
> +#include "VideoLayerChromium.h"
>  
>  #include "WebCanvas.h"
>  #include "WebCString.h"
> @@ -156,6 +157,12 @@ void WebMediaPlayerClientImpl::cancelLoad()
>      if (m_webMediaPlayer.get())
>          m_webMediaPlayer->cancelLoad();
>  }
> +#if USE(ACCELERATED_COMPOSITING)
> +WebCore::PlatformLayer* WebMediaPlayerClientImpl::platformLayer() const
> +{
> +    return m_videoLayer.get();
> +}
> +#endif
>  
>  void WebMediaPlayerClientImpl::play()
>  {
> @@ -372,6 +379,9 @@ MediaPlayerPrivateInterface* WebMediaPlayerClientImpl::create(MediaPlayer* playe
>  {
>      WebMediaPlayerClientImpl* client = new WebMediaPlayerClientImpl();
>      client->m_mediaPlayer = player;
> +#if USE(ACCELERATED_COMPOSITING)
> +    client->m_videoLayer = VideoLayerChromium::create(0);
> +#endif
>      return client;
>  }
>  
> diff --git a/WebKit/chromium/src/WebMediaPlayerClientImpl.h b/WebKit/chromium/src/WebMediaPlayerClientImpl.h
> index 57c93b7b4c5d37e4eef4e53e85704103481a85ec..a88fde15b733443ec9f43b4718a8d64d7a68684b 100644
> --- a/WebKit/chromium/src/WebMediaPlayerClientImpl.h
> +++ b/WebKit/chromium/src/WebMediaPlayerClientImpl.h
> @@ -66,6 +66,9 @@ public:
>      // MediaPlayerPrivateInterface methods:
>      virtual void load(const WebCore::String& url);
>      virtual void cancelLoad();
> +#if USE(ACCELERATED_COMPOSITING)
> +    virtual WebCore::PlatformLayer* platformLayer() const;
> +#endif
>      virtual void play();
>      virtual void pause();
>      virtual bool supportsFullscreen() const;
> @@ -94,6 +97,10 @@ public:
>      virtual void setSize(const WebCore::IntSize&);
>      virtual void paint(WebCore::GraphicsContext*, const WebCore::IntRect&);
>      virtual bool hasSingleSecurityOrigin() const;
> +#if USE(ACCELERATED_COMPOSITING)
> +    virtual bool supportsAcceleratedRendering() const { return true; }
> +#endif
> +
>      virtual WebCore::MediaPlayer::MovieLoadType movieLoadType() const;
>  
>  private:
> @@ -106,6 +113,9 @@ private:
>  
>      WebCore::MediaPlayer* m_mediaPlayer;
>      OwnPtr<WebMediaPlayer> m_webMediaPlayer;
> +#if USE(ACCELERATED_COMPOSITING)
> +    RefPtr<WebCore::PlatformLayer> m_videoLayer;
> +#endif
>      static bool m_isEnabled;
>  };
>  

WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:383
 +      client->m_videoLayer = VideoLayerChromium::create(0);
Do so only if accelerated compositing is enabled in runtime. This is a compile time flag which is true for audio all the time.

WebKit/chromium/src/WebMediaPlayerClientImpl.h:101
 +      virtual bool supportsAcceleratedRendering() const { return true; }
Do so only if accelerated compositing is enabled in runtime. This is a compile time flag which is true for audio all the time.

WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:163
 +      return m_videoLayer.get();
assert if accelerated compositing is not enabled.

[David Levin]

Comment on attachment 61534 [details]
Patch

r- Based on Alpha's feedback.

[Victoria Kirst]

Created attachment 61870 [details]
Patch

[Victoria Kirst]

Comment on attachment 61870 [details]
Patch

Made changes according to Alpha's suggestions.

[WebKit Review Bot]

Attachment 61870 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3360590

[David Levin]

Comment on attachment 61870 [details]
Patch

In addition to my (minor) comment below:
1. Please figure out the chromium build break. (You may need a DEPS roll.)
2. It would be nice to get Alpha to look it over again.

> diff --git a/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp b/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp
>  WebMediaPlayerClientImpl::WebMediaPlayerClientImpl()
>      : m_mediaPlayer(0)
> +#if USE(ACCELERATED_COMPOSITING)
> +      ,
> +      m_videoLayer(0),
> +      m_supportsAcceleratedCompositing(false)

The commas should be at the beginning of the lines (and this will happen to make the formatting more uniform).

[Hin-Chung Lam]

The code looks good to me. Just need to fix style according to David's comments.

[Victoria Kirst]

Created attachment 61969 [details]
Patch

[Victoria Kirst]

Comment on attachment 61969 [details]
Patch

Fixed formatting inconsistency with constructor and fixed the build issue (had to include a header behind a "if use(...)")

[WebKit Commit Bot]

Comment on attachment 61969 [details]
Patch

Clearing flags on attachment: 61969

Committed r63723: <http://trac.webkit.org/changeset/63723>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.