Bug 23140 – <video> poster should scale like a video frame

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Eric Carlson]

The spec says the poster image is "intended to be a representative frame of the video (typically one of the first non-blank frames) that gives the user an idea of what the video is like", so when the poster size doesn't match the element size it should be scaled like a frame of video, preserving aspect ratio.

[Eric Carlson]

Created attachment 26463 [details]
test case

[Eric Carlson]

Created attachment 46253 [details]
Revised test case

[Eric Carlson]

Created attachment 46254 [details]
Proposed patch

[WebKit Review Bot]

Attachment 46254 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/177516

[WebKit Review Bot]

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

[WebKit Review Bot]

Attachment 46254 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/177522

[Eric Carlson]

Comment on attachment 46254 [details]
Proposed patch

ack, attached the wrong version of the patch

[Darin Adler]

Comment on attachment 46254 [details]
Proposed patch

> +    bool updatePoster = false;

Even in a local variable, I find that naming data with a verb is confusing. How about shouldUpdatePoster or shouldUpdatePosterImage?

> +    virtual bool videoFrameAvailable() const { return false; }

Since this is currently called only within the HTMLMediaElement class, I suggest making it private instead of public.

> +        m_posterUrl = document()->completeURL(attr->value());

URL should be capitalized as URL, not Url. It's a little strange that we don't strip leading and trailing whitespace here the way we do for other attributes we complete. Sadly we usually do it with deprecatedParseURL, a function that does other unwanted things. Also, given the recent discussion in the HTML5 mailing list we may eventually want to special case the empty string here.

> +bool HTMLVideoElement::videoFrameAvailable() const

We normally like to name boolean members so they fit in the sentence "video element <xxx>", so it would be hasAvailableVideoFrame or hasVideoFrame.

> +    KURL poster() const { return m_posterUrl; }

This could be const KURL& instead of URL to avoid a little copying overhead.

> +    bool shouldDisplayPoster() const { return m_shouldShowPosterImage; }

Seems to me it would be better to have the function and the data member have the same name.

> +    bool m_startedPlaying : 1;
> +    bool m_isStreaming : 1;
> +    bool m_visible : 1;
> +    bool m_hasUnsupportedTracks : 1;
> +    bool m_videoFrameAvailable : 1;

Do these need to be bitfields? I like the idea of making objects smaller if there are a lot of them, but I'm not sure about this case. Don't bitfields make code bigger and a little slower? I recently convinced Adam Barth to not use bitfields in SecurityOrigin.

> +bool MediaPlayerPrivate::videoFrameAvailable() const
> +{
> +    // When using a QTMovieLayer return true as soon as the movie reaches QTMovieLoadStatePlayable 
> +    // because although we don't *know* when the first frame has decoded, by the time we get and 
> +    // process the notificaiton a frame should have propagated the VisualContext and been set on
> +    // the layer.
> +    if (currentRenderingMode() == MediaRenderingMovieLayer)
> +        return m_readyState >= MediaPlayer::HaveCurrentData;
> +
> +    // When using the software renderer QuickTime signals that a frame is available so we might as well
> +    // wait until we know that a frame has been drawn.
> +    return m_videoFrameAvailable;
> +}

It's strange to have a function member and a data member that don't mean the same thing. I suggest giving the data member a more specific name to make it clear it's not always the same as the function result.

> +void RenderImage::paint(GraphicsContext* context, const IntRect& rect)
> +{
> +    if ((document()->printing() && !view()->printImages()) || !hasImage() || errorOccurred())
> +        return;
> +
> +    if (hasImage() && rect.width() > 0 && rect.height() > 0) {
> +        Image* img = image(rect.width(), rect.height());
> +        if (!img || img->isNull())
> +            return;
> +
>          HTMLImageElement* imageElt = (node() && node()->hasTagName(imgTag)) ? static_cast<HTMLImageElement*>(node()) : 0;
>          CompositeOperator compositeOperator = imageElt ? imageElt->compositeOperator() : CompositeSourceOver;
> -        context->drawImage(image(cWidth, cHeight), style()->colorSpace(), rect, compositeOperator, useLowQualityScaling);
> +        bool useLowQualityScaling = RenderImageScaleObserver::shouldImagePaintAtLowQuality(this, rect.size());
> +        context->drawImage(image(rect.width(), rect.height()), style()->colorSpace(), rect, compositeOperator, useLowQualityScaling);
>      }
>  }

Does adding this extra work (function call overhead, redoing various checks) to every image draw have a measurable impact on performance? I'd guess not.
>  
> +    virtual void paint(GraphicsContext* context, const IntRect& rect);

I suggest making this function private, and leaving out the argument names.

> +    virtual bool isRenderImage() const { return false; }

This is not a good idea. The isRenderImage function is suppose to be a checked type cast, mechanical, so if RenderMedia is derived from RenderImage, it should return true.

If there are some call sites where we need to handle media differently, we need a new function with a new name, or additional code. Changing isRenderImage to return false seems like a bad direction.

> - * Copyright (C) 2007 Apple Inc.  All rights reserved.
> + * Copyright (C) 2007, 2010 Apple Inc.  All rights reserved.

I'm guessing we had published changes in 2008 and 2009 and forgot to include them, so you sould.

> +#include "RenderReplaced.h"

I don't understand why this is needed. Since RenderImage is derived from RenderReplaced, this header should be included already.

> +HTMLVideoElement* RenderVideo::videoElement() const
> +{ 
> +    return static_cast<HTMLVideoElement*>(node()); 
> +}

You should have an assertion here too, checking the tag of the node.

> +    HTMLVideoElement* videoElement() const;

Since this is only used inside the .cpp file you might want to mark the implementation inline. As long as nobody uses it in another translation unit you will be fine and save a function call each time.
>  
> +    virtual void imageChanged(WrappedImagePtr, const IntRect* = 0);

I don't think you need/want the "= 0" here.

> +    virtual int minimumReplacedHeight() const { return 0; }

I normally suggest not putting bodies into .h files, even if they are small.

Patch looks great, but review- because I think the isRenderImage approach here is wrong.

[Darin Adler]

Oof, reviewed wrong version of the patch!

[Eric Carlson]

Created attachment 46260 [details]
updated patch

(In reply to comment #8)

New patch addresses all comments except:

> >  
> > +    virtual void paint(GraphicsContext* context, const IntRect& rect);
> 
> I suggest making this function private, and leaving out the argument names.
> 
Dropped the arguments but it can't be private because RenderVideo call it.

Sorry for the bogus patch, luckily the one I meant to post had only a few different lines (the ones that made it compile).

Thanks for the review!

[Simon Fraser (smfr)]

I'm not sure I like RenderMedia inheriting from RenderImage. This will cause isImage() and isRenderImage() to return true for RenderMedia, possibly giving unwanted behavior.

[Darin Adler]

(In reply to comment #11)
> I'm not sure I like RenderMedia inheriting from RenderImage. This will cause
> isImage() and isRenderImage() to return true for RenderMedia, possibly giving
> unwanted behavior.

Eric's patch originally changed both isImage and isRenderImage to return false for RenderMedia. See comments above.

We need to get more concrete than that, though, and find specific examples where the wrong thing happens. Otherwise, I think the inheritance may be OK.

[Simon Fraser (smfr)]

Comment on attachment 46260 [details]
updated patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 53064)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,85 @@
> [phone]  Eric Carlson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        rdar://5684062

Should use a valid URL as copied from Radar.


> Index: WebCore/html/HTMLMediaElement.h
> ===================================================================
> --- WebCore/html/HTMLMediaElement.h	(revision 53038)
> +++ WebCore/html/HTMLMediaElement.h	(working copy)
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2007, 2008, 2009 Apple Inc. All rights reserved.
> + * Copyright (C) 2007, 2008, 2009, 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
> @@ -86,7 +86,7 @@ public:
>      MediaPlayer::MovieLoadType movieLoadType() const;
>      
>      bool inActiveDocument() const { return m_inActiveDocument; }
> -    
> +
>  // DOM API
>  // error state
>      PassRefPtr<MediaError> error() const;

Only whitespace and the copyright changed in this file.

> Index: WebCore/html/HTMLVideoElement.cpp
> ===================================================================

> +        if (m_shouldDisplayPoster) {

I have a minor preference for calling this m_shouldDisplayPosterImage

> Index: WebCore/html/HTMLVideoElement.h
> ===================================================================

> +    const KURL& poster() const { return m_posterURL; }
>      void setPoster(const String&);

Odd that the getter and setter use different types. I'd also prefer them to be named posterURL() and setPosterURL().

> Index: WebCore/loader/ImageLoader.cpp
> ===================================================================

> -        if (!renderer->isImage())
> +        if (!renderer->isImage() && !renderer->isVideo())
>              return;
>          RenderImage* imageRenderer = toRenderImage(renderer);

Maybe this should be calling toRenderImage()?

> Index: WebCore/rendering/RenderImage.h
> ===================================================================
> +    virtual void paint(GraphicsContext*, const IntRect&);

I'd prefer this be called paintIntoRect().

>  inline RenderImage* toRenderImage(RenderObject* object)
>  { 
> -    ASSERT(!object || object->isRenderImage());
> +    ASSERT(!object || object->isRenderImage() || object->isVideo());

Shouldn't need this change.

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 53064)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,17 @@
> [phone]  Eric Carlson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        rdar://5684062
> +        https://bugs.webkit.org/show_bug.cgi?id=23094
> +        Flash of white when switching from poster image to video playback
> +        
> +        https://bugs.webkit.org/show_bug.cgi?id=23140
> +        <video> poster should scale like a video frame
> +
> +        * media/video-poster-expected.txt: Remove blank line at beginning of test result present
> +        as a side effect of the media element using RenderImage to display the poster.
> +
>  2010-01-10  Daniel Bates  <[e-mail]>
>  
>          Rubber-stamped by Eric Seidel.
> Index: LayoutTests/media/video-poster-expected.txt
> ===================================================================
> --- LayoutTests/media/video-poster-expected.txt	(revision 53038)
> +++ LayoutTests/media/video-poster-expected.txt	(working copy)
> @@ -1,4 +1,3 @@
> -
>  EXPECTED (video.getAttribute('poster') == 'content/greenbox.png') OK
>  EXPECTED (relativeURL(video.poster) == 'content/greenbox.png') OK
>  EXPECTED (video.getAttribute('poster') == 'content/abe.png') OK

I'd like to see a new testcase that tests various configurations of poster image size (width- and height-constrained aspect ratio sizing of poster image).

r=me

[Darin Adler]

(In reply to comment #13)
> > +    const KURL& poster() const { return m_posterURL; }
> >      void setPoster(const String&);
> 
> Odd that the getter and setter use different types. I'd also prefer them to be
> named posterURL() and setPosterURL().

I believe both aspects of this, the type and the name, are standard for URL-type HTML attributes like the <video> element poster.

[Eric Carlson]

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