Currently if you have a video which is a different size than its width and height attributes specify, it's scaled WRT its aspect ratio to fit in its block. We don't scale video's poster WRT the aspect ratio though, we distort it to fill the width x height block. We should scale poster WRT aspect ratio. If we don't, we can't (for example) take a screen shot of a scaled video playing and used that as the poster attribute, as it will be distorted when it's scaled. We should get the HTML spec changed as well for this.

FWIW, the aspect ratio is preserved on Chrome, Safari (Windows and iOS), and Opera. IE9 is the only other browser I tested that behaves like FF.

Could we please get a minimized testcase attached to this bug?

When loading the page, you will see the poster image. It should be pillar-boxed, the same as the video when you hit play. Instead, the image is stretched to fill the box. There should be a circle in the box, but it is stretched to an oval.

Bug confirmed on Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0 Can a developer look into what might be happening here?

So This "fixes" the issue, but it's pretty ugly looking. It definitely needs to be cleaned up. Any suggestions/best practices on this?

Comment on attachment 595655 [details] [diff] [review] Ugly hack to get it working gfx/layout stuff is more Roc's area than mine. cadecairos: It would be helpful if you can also describe what the old code was doing wrong, and how this patch fixes the problem.

cpearce: Thanks for the tip! So, before my patch, this bit of code was the problem(lines 338-341 in nsVideoFrame.cpp): if (ShouldDisplayPoster()) { kidReflowState.SetComputedWidth(aReflowState.ComputedWidth()); kidReflowState.SetComputedHeight(aReflowState.ComputedHeight()); It tells the frame that it should paint the image so it fills 100% of the frames width and height. This bit here(lines 345-349): ReflowChild(imageFrame, aPresContext, kidDesiredSize, kidReflowState, mBorderPadding.left, mBorderPadding.top, 0, aStatus); FinishReflowChild(imageFrame, aPresContext, &kidReflowState, kidDesiredSize, mBorderPadding.left, mBorderPadding.top, 0); finishes the reflow. Important to note here are the mBorderPadding.left, and mBorderPadding.right parameters, which set the offsets of the image from the left and the top of the frame). I'll go through what I've added: + PRUint32 posterHeight, posterWidth; + PRInt32 videoElemHeight, videoElemWidth; + gfxSize scaledRatio; This sets up some variables I need. Annoyingly, the GetNaturalHeight method for images returns a unsigned int and the GetWidth method for the video element accepts an unsigned integer. I use the gfxSize object to compute the size of the element according to its ratio. + nsCOMPtr<nsIDOMHTMLImageElement> posterImage = do_QueryInterface(mPosterImage); + NS_ENSURE_TRUE(posterImage, NS_ERROR_FAILURE); + posterImage->GetNaturalHeight( &posterHeight ); + posterImage->GetNaturalWidth( &posterWidth ); This bit just gets an interface to the anonymous image element, and gets it's natural height and width in pixels. + nsCOMPtr<nsIDOMHTMLVideoElement> videoDomElement = do_QueryInterface(mContent); + videoDomElement->GetWidth( &videoElemWidth ); + videoDomElement->GetHeight( &videoElemHeight ); Similar to above, I get an interface to the video element and grab its height and width in pixels. + if ( !ShouldDisplayPoster() || !posterHeight || !posterWidth || !videoElemHeight || !videoElemWidth ) { kidReflowState.SetComputedWidth(0); kidReflowState.SetComputedHeight(0); Check if anything is NULL or 0, if it is the computed width will be set to 0 (nothing to display) + } else { + gfxFloat scale = + NS_MIN(static_cast<float>(videoElemWidth)/posterWidth, + static_cast<float>(videoElemHeight)/posterHeight); I "borrowed" this bit from CorrectForAspectRatio. It calculates the scale on which to resize the height and width and grabs the lesser of the two. The reason I have to cast to float is so that I get a real number representing the actual scale. + scaledRatio = gfxSize(scale*posterWidth, + scale*posterHeight); I'm not entirely convinced I need this, but right here I store the scaled pixel height and width in a gfxSize object. + kidReflowState.SetComputedWidth(nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.width))); + kidReflowState.SetComputedHeight(nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.height))); Right here I take the calculated height and width and pass them into this handy method which converts the pixels to AppUnits. I use the calculated values to tell the frame what size the poster image should be. ReflowChild(imageFrame, aPresContext, kidDesiredSize, kidReflowState, + nsPresContext::CSSPixelsToAppUnits((videoElemWidth - static_cast<PRInt32>(scaledRatio.width))/2), + nsPresContext::CSSPixelsToAppUnits((videoElemHeight - static_cast<PRInt32>(scaledRatio.height))/2), 0, aStatus); I replaced the mBorderPadding values here with a calculation that determines the left and top position to draw the image at.

Comment on attachment 595655 [details] [diff] [review] Ugly hack to get it working Review of attachment 595655 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsVideoFrame.cpp @@ +347,5 @@ > + posterImage->GetNaturalWidth( &posterWidth ); > + > + nsCOMPtr<nsIDOMHTMLVideoElement> videoDomElement = do_QueryInterface(mContent); > + videoDomElement->GetWidth( &videoElemWidth ); > + videoDomElement->GetHeight( &videoElemHeight ); This just gets the values from the DOM attributes (e.g., ignoring the effect of CSS rules). I don't understand why the layout of the poster image needs to depend on these attributes.

I pushed the patch to TryServer, and got reftest failures: https://tbpl.mozilla.org/?tree=Try&rev=52c122fd8e38 REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-1.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-7.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-10.html | image comparison (==) Bug 721014 - Intermittent REFTEST TEST-UNEXPECTED-FAIL | ogg-video/poster-10.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-11.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-12.html | image comparison (==) Bug 681170 - aspect-ratio-1a.xhtml, canvas-1b.xhtml, aspect-ratio-3a.xhtml, aspect-ratio-3b.xhtml, aspect-ratio-1b.xhtml, canvas-1a.xhtml, poster-12.html | assertion count 1 is more than expected 0 "ASSERTION: RECURSION_LEVEL(table_) > 0, file pldhash.cpp, line 707" REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-13.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-15.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-1.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-7.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-10.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-11.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-12.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-13.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-15.html | image comparison (==)

(In reply to Chris Pearce (:cpearce) from comment #10) > I pushed the patch to TryServer, and got reftest failures: > https://tbpl.mozilla.org/?tree=Try&rev=52c122fd8e38 Looks like these are happening because my patch breaks if a height and width on the element isn't specified, it won't draw the poster. in reply to Comment 9, I don't think it should depend on those attributes. The reftest failures make that clear. How can I determine the actual height and width of the video element?

When nsVideoFrame::Reflow is called, the computed width and height in aReflowState are the final width/height for the frame (which is why we set aMetrics.width/height to them at the beginning). So you just need to size and position the poster-image frame so it's letterboxed within (0,0,aMetrics.width,aMetrics.height).

If I have a video that's 300x400 and a poster image that's 150x200, and My video element has no explicit height and width, the element becomes the size of the small poster image and grows to the size of the video after playback begins. The reverse is true if the poster image is larger than the video is (shrinks after playback begins). Is this expected behavior? I would think the image should be scaled to the size of the video in the event there's no height and width.

(In reply to Chris DeCairos (:cadecairos) from comment #13) > If I have a video that's 300x400 and a poster image that's 150x200, and My > video element has no explicit height and width, the element becomes the size > of the small poster image and grows to the size of the video after playback > begins. The reverse is true if the poster image is larger than the video is > (shrinks after playback begins). Is this expected behavior? Apparently not: http://www.whatwg.org/specs/web-apps/current-work/#dom-video-videowidth "The intrinsic width of a video element's playback area is the intrinsic width of the video resource, if that is available; otherwise it is the intrinsic width of the poster frame, if that is available; otherwise it is 300 CSS pixels." But I think that's a separate bug in nsVideoFrame::GetVideoIntrinsicSize. Don't worry about it in this bug. Feel free to file a bug on the sizing issue and fix that separately :-).

Great, I'll do just that. Although that problem may be why the reftests that cpearce reported were failing. I can't confirm that claim just yet though..

I've changed my patch to use aMetrics, which seems to work great. A couple reftests are broken, and I think the best way to "fix" them is to apply this patch on top of my work in Bug 726904, which also affects the same tests. I need to check if the reftests will pass with both the new behaviours present.

Comment on attachment 615612 [details] [diff] [review] Modified my original patch to use aMetrics Review of attachment 615612 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsVideoFrame.cpp @@ +285,5 @@ > aMetrics.width, > aMetrics.height); > + > + PRUint32 posterHeight, posterWidth; > + gfxSize scaledRatio; Move the definition down below. Instead of saving scaledRatio for use later, save an nsSize which is the size after converting to appunits, so you don't have to keep repeating that. @@ +292,5 @@ > + NS_ENSURE_TRUE(posterImage, NS_ERROR_FAILURE); > + posterImage->GetNaturalHeight( &posterHeight ); > + posterImage->GetNaturalWidth( &posterWidth ); > + > + if ( !ShouldDisplayPoster() || !posterHeight || !posterWidth || !aMetrics.height || !aMetrics.width ) { Line too long, break it. Also, lose spaces after ( and before ). Don't need to check aMetrics/width/height here. @@ +298,5 @@ > kidReflowState.SetComputedHeight(0); > + } else { > + gfxFloat scale = > + NS_MIN(static_cast<float>(aMetrics.width)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterWidth)), > + static_cast<float>(aMetrics.height)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterHeight))); Casts to PRInt32 should not be needed. @@ +300,5 @@ > + gfxFloat scale = > + NS_MIN(static_cast<float>(aMetrics.width)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterWidth)), > + static_cast<float>(aMetrics.height)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterHeight))); > + scaledRatio = gfxSize(scale*posterWidth, > + scale*posterHeight); This is where you should declare scaledRatio. You can write it as "gfxSize scaledRatio(scale*posterWidth, scale*posterHeight)". @@ +302,5 @@ > + static_cast<float>(aMetrics.height)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterHeight))); > + scaledRatio = gfxSize(scale*posterWidth, > + scale*posterHeight); > + kidReflowState.SetComputedWidth(nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.width))); > + kidReflowState.SetComputedHeight(nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.height))); Don't need these PRInt32 casts. @@ +311,4 @@ > FinishReflowChild(imageFrame, aPresContext, > &kidReflowState, kidDesiredSize, > + (aMetrics.width - nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.width)))/2, > + (aMetrics.height - nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.height)))/2, 0); Compute the x/y once, store in an nsPoint, so you don't have to repeat these expressions.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17) > @@ +298,5 @@ > > kidReflowState.SetComputedHeight(0); > > + } else { > > + gfxFloat scale = > > + NS_MIN(static_cast<float>(aMetrics.width)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterWidth)), > > + static_cast<float>(aMetrics.height)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterHeight))); > > Casts to PRInt32 should not be needed. > > @@ +302,5 @@ > > + static_cast<float>(aMetrics.height)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterHeight))); > > + scaledRatio = gfxSize(scale*posterWidth, > > + scale*posterHeight); > > + kidReflowState.SetComputedWidth(nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.width))); > > + kidReflowState.SetComputedHeight(nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.height))); > > Don't need these PRInt32 casts. I cast these variables here because CSSPixelsToAppUnits accepts either a 32 bit int or a float ( posterWidth/height are unsigned ints, and scaledRatio.height/width are doubles ) is there another way of doing this?

Cast to a float so the results doesn't have to be rounded to an integer.

This patch is based on the patch for Bug 726904. I've made all the changes from roc's review, and a couple more based on my fixes. I've also made the necessary changes to reftests that used to assume there was no poster frame scaling. here's my tbpl push for this patch (includes 726904): https://tbpl.mozilla.org/?tree=Try&rev=2fde97b75aa7

Comment on attachment 616766 [details] [diff] [review] Changes to patch, as per review Review of attachment 616766 [details] [diff] [review]: ----------------------------------------------------------------- What is poster-ref-blue125x100.html for? Nothing seems to use it. ::: layout/generic/nsVideoFrame.cpp @@ +287,5 @@ > aMetrics.height); > + > + PRUint32 posterHeight, posterWidth; > + nsSize AUSize(0, 0), > + computedArea(aReflowState.ComputedWidth(), aReflowState.ComputedHeight()); AUSize is not a very meaningful name. Call it scaledPosterSize. @@ +288,5 @@ > + > + PRUint32 posterHeight, posterWidth; > + nsSize AUSize(0, 0), > + computedArea(aReflowState.ComputedWidth(), aReflowState.ComputedHeight()); > + nsPoint reflowSize(0, 0); Call this posterTopLeft @@ +293,5 @@ > + > + nsCOMPtr<nsIDOMHTMLImageElement> posterImage = do_QueryInterface(mPosterImage); > + NS_ENSURE_TRUE(posterImage, NS_ERROR_FAILURE); > + posterImage->GetNaturalHeight( &posterHeight ); > + posterImage->GetNaturalWidth( &posterWidth ); drop spaces after ( and before ) @@ +298,5 @@ > + > + if (ShouldDisplayPoster() && posterHeight && posterWidth) { > + gfxFloat scale = > + NS_MIN(static_cast<float>(computedArea.width)/nsPresContext::CSSPixelsToAppUnits(static_cast<float>(posterWidth)), > + static_cast<float>(computedArea.height)/nsPresContext::CSSPixelsToAppUnits(static_cast<float>(posterHeight))); You don't need to case posterWidth/posterHeight to float here. ::: layout/reftests/ogg-video/poster-ref-blue400x300.html @@ +1,4 @@ > <!DOCTYPE HTML> > <html> > <body style="background:white;"> > +<img src="blue250x200.png" style="position: absolute; width: 375px; height: 300px; left: 21px;"> left:21px is a bit magical. I guess it's (400 - (250/200)*300)/2 + 8, rounded (8 is the default margin-left). It would be better to use position:relative so that the results don't depend on the default margin. Also, it would be good to adjust sizes so that no rounding is required.

unfortunately, nightly won't build without those casts to float. I've got a patch coming soon.

poster-ref-blue125x100.html is used in the poster-10.html reftest in layout/reftests/ogg-video and layout/reftests/webm-video

All changes were made, except those relating to my two above comments.

Comment on attachment 622629 [details] [diff] [review] Review fixes Review of attachment 622629 [details] [diff] [review]: ----------------------------------------------------------------- Attach a new patch with that fixed and we'll check it in! Thanks! ::: layout/generic/nsVideoFrame.cpp @@ +287,5 @@ > aMetrics.height); > + > + PRUint32 posterHeight, posterWidth; > + nsSize scaledPosterSize(0, 0), > + computedArea(aReflowState.ComputedWidth(), aReflowState.ComputedHeight()); Declare computedArea with a separate nsSize declaration.

All done :) This patch needs to land on top of Bug 726904 How can we ensure this happens?

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=cfb9b2418c2b