[Webkit-unassigned] [Bug 39557] full screen doesn't work for video elements
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun May 23 20:03:47 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=39557
--- Comment #11 from Chris Marrin <cmarrin at apple.com> 2010-05-23 20:03:47 PST ---
(From update of attachment 56830)
This is an unofficial review - no status change
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 60048)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,31 @@
> +2010-05-22 Jer Noble <jer.noble at apple.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + full screen doesn't work for video elements
> + https://bugs.webkit.org/show_bug.cgi?id=39557
> + rdar://problem/8011813
> +
> + Add fullscreen support for MediaPlayerPrivateVisualContext. A new class, MediaPlayerPrivateFullscreenWindow,
> + provides the fullscreen hwnd and layer renderer. Any WKCACFLayer can be provided to MediaPlayerPrivateFullscreenWindow
> + so future additional MediaPlayerPrivate implementations can use the fullscreen window.
> +
> + Minor additions have been made to the FloatSize and IntSize classes.
It would be nice to note the reason for the changes
> Index: WebCore/platform/graphics/FloatSize.h
> ===================================================================
> --- WebCore/platform/graphics/FloatSize.h (revision 60048)
> +++ WebCore/platform/graphics/FloatSize.h (working copy)
> @@ -63,6 +63,14 @@ public:
>
> bool isEmpty() const { return m_width <= 0 || m_height <= 0; }
>
> + float aspectRatio() const { return m_width / m_height; }
You should zero check m_height here to avoid divide by 0
> Index: WebCore/platform/graphics/IntSize.h
> ===================================================================
> --- WebCore/platform/graphics/IntSize.h (revision 60048)
> +++ WebCore/platform/graphics/IntSize.h (working copy)
> @@ -72,6 +72,8 @@ public:
>
> bool isEmpty() const { return m_width <= 0 || m_height <= 0; }
> bool isZero() const { return !m_width && !m_height; }
> +
> + float aspectRatio() const { return static_cast<float>(m_width) / static_cast<float>(m_height); }
Same as above
> Index: WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp
> ===================================================================
>
> +void MediaPlayerPrivateFullscreenWindow::createWindow()
> +{
> + static ATOM windowAtom = 0;
> + if (!windowAtom) {
> + WNDCLASSEX wcex = {0};
> + wcex.cbSize = sizeof(WNDCLASSEX);
> + wcex.style = CS_HREDRAW | CS_VREDRAW | CS_OWNDC;
> + wcex.lpfnWndProc = &staticWndProc;
> + wcex.hInstance = WebCore::instanceHandle();
> + wcex.lpszClassName = L"MediaPlayerPrivateFullscreenWindowClass";
> + windowAtom = ::RegisterClassEx(&wcex);
WebKit style is to not align equal signs, I believe
> + }
> +
> + if (m_hwnd)
> + close();
> +
> + MONITORINFO mi = {0};
> + mi.cbSize = sizeof(MONITORINFO);
> + GetMonitorInfo(MonitorFromWindow(0, MONITOR_DEFAULTTOPRIMARY), &mi);
> + IntRect monitorRect = mi.rcMonitor;
> +
> + CREATESTRUCTW cs = {0};
> + cs.x = 0;
> + cs.y = 0;
> + cs.cx = monitorRect.width();
> + cs.cy = monitorRect.height();
> + cs.style = WS_POPUP | WS_VISIBLE;
> + cs.dwExStyle = 0;
> + cs.lpszClass = L"MediaPlayerPrivateFullscreenWindowClass";
> + cs.lpszName = L"";
> + cs.lpCreateParams = (LPVOID)this;
> +
> + m_hwnd = ::CreateWindowExW(cs.dwExStyle,
> + cs.lpszClass,
> + cs.lpszName,
> + cs.style,
> + cs.x,
> + cs.y,
> + cs.cx,
> + cs.cy,
> + cs.hwndParent,
> + cs.hMenu,
> + cs.hInstance,
> + cs.lpCreateParams);
Webkit style is also to put params on one line, only splitting when lines are really long
> +}
> +
> +void MediaPlayerPrivateFullscreenWindow::close()
> +{
> + ::DestroyWindow(m_hwnd);
> + m_hwnd = 0;
> +}
> +
> +HWND MediaPlayerPrivateFullscreenWindow::hwnd() const
> +{
> + return m_hwnd;
> +}
> +
> +WKCACFLayerRenderer* MediaPlayerPrivateFullscreenWindow::layerRenderer()
> +{
> + return m_layerRenderer.get();
> +}
This method can be const
> +
> +WKCACFLayer* MediaPlayerPrivateFullscreenWindow::rootChildLayer()
> +{
> + return m_rootChild.get();
> +}
As can this one.
> +
> +void MediaPlayerPrivateFullscreenWindow::setRootChildLayer(PassRefPtr<WKCACFLayer> rootChild)
> +{
> + if (m_rootChild != rootChild) {
> +
> + if (m_rootChild) {
> + m_rootChild->removeFromSuperlayer();
> + m_rootChild.clear();
> + }
> +
> + m_rootChild = rootChild;
> + }
> +
> + if (m_rootChild) {
> + m_layerRenderer->setRootChildLayer(m_rootChild.get());
> + WKCACFLayer* rootLayer = m_rootChild->rootLayer();
> + CGRect rootBounds = m_rootChild->rootLayer()->bounds();
> + m_rootChild->setFrame(rootBounds);
WKCACFLayer no longer has a setFrame() API. I removed it to simplify the API. Frame is always so confusing because it's nothing more than a composite of bounds.size and position. So you can replace this with calls to setBounds and setPosition.
> + m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin), IntSize(rootBounds.size));
> + m_rootChild->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
> + rootLayer->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
The background of the rootChild should never be seen (always covered by the video) right? You might want to make it's background red (at least in debug mode) to make it easier to spot problems. But maybe video needs to show the background before it's started rendering?
> + }
> +}
Since you're doing fullscreen video with a fullscreen window, it seems like you should be able to animate it like we do on Mac right? Not for now of course, but is there a bug open about this?
> +
> +LRESULT CALLBACK MediaPlayerPrivateFullscreenWindow::staticWndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
> +{
> + LONG_PTR longPtr = GetWindowLongPtr(hWnd, GWLP_USERDATA);
> +
> + if (!longPtr && message == WM_CREATE) {
> + LPCREATESTRUCT lpcs = reinterpret_cast<LPCREATESTRUCT>(lParam);
> + longPtr = (LONG_PTR)lpcs->lpCreateParams;
> + ::SetWindowLongPtr(hWnd, GWLP_USERDATA, longPtr);
> + }
> +
> + if (MediaPlayerPrivateFullscreenWindow* window = reinterpret_cast<MediaPlayerPrivateFullscreenWindow*>(longPtr))
> + return window->wndProc(hWnd, message, wParam, lParam);
> +
> + return ::DefWindowProc(hWnd, message, wParam, lParam);
> +}
> +
> +LRESULT MediaPlayerPrivateFullscreenWindow::wndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
> +{
> + LRESULT lResult = 0;
> + switch (message) {
> + case WM_CREATE:
> + m_hwnd = hWnd;
> + m_layerRenderer->setHostWindow(m_hwnd);
> + m_layerRenderer->createRenderer();
> + m_layerRenderer->setNeedsDisplay();
> + break;
> + case WM_DESTROY:
> + m_hwnd = 0;
> + m_layerRenderer->destroyRenderer();
> + m_layerRenderer->setHostWindow(0);
> + case WM_WINDOWPOSCHANGED:
> + {
> + LPWINDOWPOS wp = (LPWINDOWPOS)lParam;
> + if (!(wp->flags & SWP_NOMOVE)) {
> + m_layerRenderer->resize();
> + WKCACFLayer* rootLayer = m_rootChild->rootLayer();
> + CGRect rootBounds = m_rootChild->rootLayer()->bounds();
> + m_rootChild->setFrame(rootBounds);
Same as above
> + m_rootChild->setNeedsLayout();
> + m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin), IntSize(rootBounds.size));
> + }
> + }
> + break;
> + case WM_PAINT:
> + m_layerRenderer->renderSoon();
> + break;
> + }
> +
> + if (m_client)
> + lResult = m_client->fullscreenClientWndProc(hWnd, message, wParam, lParam);
> +
> + return lResult;
> +}
> +
> +}
> Index: WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h
> ===================================================================
> --- WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h (revision 0)
> +++ WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h (revision 0)
> @@ -0,0 +1,68 @@
> +/*
> + * Copyright (C) 2010 Apple 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:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. 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.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS 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 APPLE INC. OR ITS 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 MediaPlayerPrivateFullscreenWindow_h
> +#define MediaPlayerPrivateFullscreenWindow_h
> +
> +#include "WKCACFLayer.h"
> +#include "WKCACFLayerRenderer.h"
> +#include <windows.h>
> +#include <wtf/OwnPtr.h>
> +
> +namespace WebCore {
> +
> +class MediaPlayerPrivateFullscreenClient {
> +public:
> + virtual LRESULT fullscreenClientWndProc(HWND, UINT message, WPARAM, LPARAM) = 0;
> +};
> +
> +class MediaPlayerPrivateFullscreenWindow {
> +public:
> + MediaPlayerPrivateFullscreenWindow(MediaPlayerPrivateFullscreenClient*);
> + virtual ~MediaPlayerPrivateFullscreenWindow();
> +
> + void createWindow();
> + void close();
> +
> + HWND hwnd() const;
> +
> + WKCACFLayerRenderer* layerRenderer();
> +
> + WKCACFLayer* rootChildLayer();
Const for these two
> + void setRootChildLayer(PassRefPtr<WKCACFLayer>);
> +
> +protected:
> + static LRESULT CALLBACK staticWndProc(HWND, UINT message, WPARAM, LPARAM);
> + LRESULT wndProc(HWND, UINT message, WPARAM, LPARAM);
> +
> + MediaPlayerPrivateFullscreenClient* m_client;
> + OwnPtr<WKCACFLayerRenderer> m_layerRenderer;
> + RefPtr<WKCACFLayer> m_rootChild;
> + HWND m_hwnd;
> +};
> +
> +}
> +
> +#endif
> Index: WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp
> ===================================================================
> --- WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp (revision 60048)
> +++ WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp (working copy)
> @@ -150,7 +150,7 @@ MediaPlayerPrivateQuickTimeVisualContext
>
> bool MediaPlayerPrivateQuickTimeVisualContext::supportsFullscreen() const
> {
> - return false;
> + return true;
> }
>
> PlatformMedia MediaPlayerPrivateQuickTimeVisualContext::platformMedia() const
> @@ -765,7 +765,7 @@ void MediaPlayerPrivateQuickTimeVisualCo
>
> buffer.unlockBaseAddress();
> layer->rootLayer()->setNeedsRender();
> - }
> + }
This looks odd. Why the change in indent? Hard to see whether or not it's right out of context
> } else
> #endif
> m_player->repaint();
> @@ -939,6 +939,10 @@ void MediaPlayerPrivateQuickTimeVisualCo
> #endif
> // The layer will get hooked up via RenderLayerBacking::updateGraphicsLayerConfiguration().
> #endif
> +
> + // fill the newly created layer with image data
> + if (m_visualContext)
> + retrieveCurrentImage();
> }
>
> void MediaPlayerPrivateQuickTimeVisualContext::destroyLayerForMovie()
> Index: WebCore/platform/graphics/win/WKCACFLayer.cpp
> ===================================================================
> --- WebCore/platform/graphics/win/WKCACFLayer.cpp (revision 60048)
> +++ WebCore/platform/graphics/win/WKCACFLayer.cpp (working copy)
> @@ -154,6 +154,7 @@ PassRefPtr<WKCACFLayer> WKCACFLayer::cre
>
> WKCACFLayer::WKCACFLayer(LayerType type)
> : m_layer(AdoptCF, CACFLayerCreate(toCACFLayerType(type)))
> + , m_layoutClient(0)
> , m_needsDisplayOnBoundsChange(false)
> {
> CACFLayerSetUserData(layer(), this);
> @@ -286,11 +287,10 @@ void WKCACFLayer::adoptSublayers(WKCACF
> void WKCACFLayer::removeFromSuperlayer()
> {
> WKCACFLayer* superlayer = this->superlayer();
> - if (!superlayer)
> - return;
> -
> CACFLayerRemoveFromSuperlayer(layer());
> - superlayer->setNeedsCommit();
> +
> + if (superlayer)
> + superlayer->setNeedsCommit();
> }
>
> WKCACFLayer* WKCACFLayer::internalSublayerAtIndex(int index) const
> @@ -343,6 +343,19 @@ void WKCACFLayer::setBounds(const CGRect
> setNeedsDisplay();
> }
>
> +void WKCACFLayer::setFrame(const CGRect& rect)
> +{
> + CGRect oldFrame = frame();
> + if (CGRectEqualToRect(rect, oldFrame))
> + return;
> +
> + CACFLayerSetFrame(layer(), rect);
> + setNeedsCommit();
> +
> + if (m_needsDisplayOnBoundsChange)
> + setNeedsDisplay();
> +}
> +
Ah, I see. You've added back setFrame! I'd really like to leave it out if we can.
> void WKCACFLayer::setContentsGravity(ContentsGravityType type)
> {
> CACFLayerSetContentsGravity(layer(), toCACFContentsGravityType(type));
> @@ -418,6 +431,32 @@ void WKCACFLayer::internalSetNeedsDispla
> CACFLayerSetNeedsDisplay(layer(), dirtyRect);
> }
>
> +void WKCACFLayer::setLayoutClient(WKCACFLayerLayoutClient* layoutClient)
> +{
> + if (layoutClient == m_layoutClient)
> + return;
> +
> + m_layoutClient = layoutClient;
> + CACFLayerSetLayoutCallback(layer(), m_layoutClient ? &layoutSublayersProc : 0);
> +}
> +
> +void WKCACFLayer::layoutSublayersProc(CACFLayerRef caLayer)
> +{
> + WKCACFLayer* layer = WKCACFLayer::layer(caLayer);
> + if (layer && layer->m_layoutClient)
> + layer->m_layoutClient->layoutSublayersOfLayer(layer);
> +}
> +
> +WKCACFLayerLayoutClient* WKCACFLayer::layoutClient() const
> +{
> + return m_layoutClient;
> +}
> +
> +void WKCACFLayer::setNeedsLayout()
> +{
> + CACFLayerSetNeedsLayout(layer());
> +}
> +
> #ifndef NDEBUG
> static void printIndent(int indent)
> {
> @@ -500,9 +539,9 @@ void WKCACFLayer::printLayer(int indent)
> CGImageRef layerContents = contents();
> if (layerContents) {
> printIndent(indent + 1);
> - fprintf(stderr, "(contents (image [%d %d]))\n",
> + fprintf(stderr, "(contents (image [%d %d]))\n",
> CGImageGetWidth(layerContents), CGImageGetHeight(layerContents));
> - }
> + }
More odd indenting
>
> // Print sublayers if needed
> int n = sublayerCount();
> Index: WebCore/platform/graphics/win/WKCACFLayer.h
> ===================================================================
> --- WebCore/platform/graphics/win/WKCACFLayer.h (revision 60048)
> +++ WebCore/platform/graphics/win/WKCACFLayer.h (working copy)
> @@ -44,9 +44,15 @@
>
> namespace WebCore {
>
> +class WKCACFLayer;
> class WKCACFAnimation;
> class WKCACFTimingFunction;
Hmmm. I see spurious class declarations here (obviously from when I first implemented this). Probably best to get rid of them while we're here.
>
> +class WKCACFLayerLayoutClient {
> +public:
> + virtual void layoutSublayersOfLayer(WKCACFLayer*) = 0;
> +};
> +
> class WKCACFLayer : public RefCounted<WKCACFLayer> {
> public:
> enum LayerType { Layer, TransformLayer };
> @@ -63,6 +69,10 @@ public:
>
> virtual void drawInContext(PlatformGraphicsContext*) { }
>
> + virtual void setLayoutClient(WKCACFLayerLayoutClient*);
> + virtual WKCACFLayerLayoutClient* layoutClient() const;
> + virtual void setNeedsLayout();
> +
> void setNeedsDisplay(const CGRect* dirtyRect = 0)
> {
> internalSetNeedsDisplay(dirtyRect);
> @@ -173,6 +183,9 @@ public:
> void setFilters(CFArrayRef filters) { CACFLayerSetFilters(layer(), filters); setNeedsCommit(); }
> CFArrayRef filters() const { return CACFLayerGetFilters(layer()); }
>
> + void setFrame(const CGRect&);
> + CGRect frame() const { return CACFLayerGetFrame(layer()); }
It would be really great if we could axe this.
> +
> void setHidden(bool hidden) { CACFLayerSetHidden(layer(), hidden); setNeedsCommit(); }
> bool isHidden() const { return CACFLayerIsHidden(layer()); }
>
> @@ -260,8 +273,11 @@ protected:
> void printLayer(int indent) const;
> #endif
>
> + static void layoutSublayersProc(CACFLayerRef);
> +
> private:
> RetainPtr<CACFLayerRef> m_layer;
> + WKCACFLayerLayoutClient* m_layoutClient;
> bool m_needsDisplayOnBoundsChange;
> };
>
> Index: WebCore/platform/graphics/win/WebTiledLayer.cpp
> ===================================================================
> --- WebCore/platform/graphics/win/WebTiledLayer.cpp (revision 60048)
> +++ WebCore/platform/graphics/win/WebTiledLayer.cpp (working copy)
> @@ -73,6 +73,15 @@ void WebTiledLayer::setBounds(const CGRe
> updateTiles();
> }
>
> +void WebTiledLayer::setFrame(const CGRect& rect)
> +{
> + if (CGRectEqualToRect(rect, frame()))
> + return;
> +
> + WebLayer::setFrame(rect);
> + updateTiles();
> +}
> +
See above
> void WebTiledLayer::internalSetNeedsDisplay(const CGRect* dirtyRect)
> {
> // FIXME: Only setNeedsDisplay for tiles that are currently visible
> Index: WebCore/platform/graphics/win/WebTiledLayer.h
> ===================================================================
> --- WebCore/platform/graphics/win/WebTiledLayer.h (revision 60048)
> +++ WebCore/platform/graphics/win/WebTiledLayer.h (working copy)
> @@ -39,6 +39,7 @@ public:
> virtual ~WebTiledLayer();
>
> virtual void setBounds(const CGRect&);
> + virtual void setFrame(const CGRect&);
See above
>
> protected:
> WebTiledLayer(const CGSize& tileSize, GraphicsLayer* owner);
> Index: WebKit/win/ChangeLog
> ===================================================================
> --- WebKit/win/ChangeLog (revision 60027)
> +++ WebKit/win/ChangeLog (working copy)
> @@ -1,3 +1,24 @@
> +2010-05-22 Jer Noble <jer.noble at apple.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + full screen doesn't work for video elements
Capitalize the start of sentences
> + https://bugs.webkit.org/show_bug.cgi?id=39557
> + rdar://problem/8011813
> +
> + Modified FullscreenVideoController to work with MediaPlayerPrivateFullscreenWindow. The FullscreenVideoController
> + is now MediaPlayerPrivate agnostic.
> +
> + * FullscreenVideoController.cpp:
> + (FullscreenVideoController::LayoutClient::LayoutClient):
> + (FullscreenVideoController::LayoutClient::layoutSublayersOfLayer):
> + (FullscreenVideoController::FullscreenVideoController):
> + (FullscreenVideoController::~FullscreenVideoController):
> + (FullscreenVideoController::enterFullscreen):
> + (FullscreenVideoController::exitFullscreen):
> + (FullscreenVideoController::createHUDWindow):
> + * FullscreenVideoController.h:
> +
> 2010-05-21 Steve Block <steveblock at google.com>
>
> Reviewed by Jeremy Orlow.
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog (revision 60028)
> +++ LayoutTests/ChangeLog (working copy)
> @@ -1,3 +1,17 @@
> +2010-05-22 Jer Noble <jer.noble at apple.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Re-enabled fullscreen support on windows, and modified tests to match.
You should add the bug number here like in the other Changelog
> +
> + * platform/win/media/controls-after-reload-expected.txt:
> + * platform/win/media/controls-drag-timebar-expected.txt:
> + * platform/win/media/controls-strict-expected.txt:
> + * platform/win/media/controls-styling-expected.txt:
> + * platform/win/media/video-controls-rendering-expected.txt:
> + * platform/win/media/video-display-toggle-expected.txt:
> + * platform/win/media/video-no-audio-expected.txt:
> +
> 2010-05-22 Maciej Stachowiak <mjs at apple.com>
>
> Reviewed by Dan Bernstein.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list