Closed Bug 726904 Opened 12 years ago Closed 12 years ago

nsVideoFrame::GetVideoIntrinsicSize setting wrong size

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: cade, Assigned: cade)

References

Details

Attachments

(1 file, 12 obsolete files)

fixed failing tests on OSX carrying forward r=cpearce
23.51 KB, patch
cade
: review+
Details | Diff | Splinter Review 
according to 
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.
The intrinsic height of a video element's playback area is the intrinsic height of the video resource, if that is available; otherwise it is the intrinsic height of the poster frame, if that is available; otherwise it is 150 CSS pixels."

nsVideoFrame::GetVideoIntrinsicSize will set the video size to the Poster Frame  even if the video size and width might be available.

Shouldn't be too hard to fix. I hope there aren't any reftests that this will break.
Assignee: nobody → chris
Status: NEW → ASSIGNED
To correct myself:

nsVideoFrame::GetVideoIntrinsicSize will set the video size to the Poster Frame  even if the video's intrinsic height and width might be available.
This might be correct but I'd like to record it here:

When GetVideoIntrinsicSize calls GetVideoSize on a video element that has preload="none" specified, it's going to return it's default values: nsSize(300,150). If the element is preload="auto" it will return the correct dimensions.

This seems like expected behavior and is the reason that there are fallback procedures in the spec. We can't expect to know the size of the video if we don't have any metadata. This would make it a logical next step to use the defaults if there is no Poster.
All I've done here is move around the code so that it will return the video size, if it's available, over the poster image size.

This will most likely break reftests
Attachment #597238 - Flags: feedback?(roc)
Comment on attachment 597238 [details] [diff] [review]
Makes GetVideoIntrinsicSize prefer video size over poster size

Review of attachment 597238 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsVideoFrame.cpp
@@ +527,2 @@
>    if (!HasVideoElement()) {
> +    if (ShouldDisplayPoster()) {

Why are you looking at the poster image only if this is *not* a video element? That seems wrong.

You need to check whether there is a video element and it has found an intrinsic size for the resource. If so, use it, otherwise check for a poster and use its size. The code should look like the spec.

@@ +545,3 @@
>  
> +    nsHTMLVideoElement* element = static_cast<nsHTMLVideoElement*>(GetContent());
> +    size = element->GetVideoSize(size);

this size is never used since we return something else below
Attachment #597238 - Flags: feedback?(roc) → feedback-
I've taken into account your feedback. How does this look?
Attachment #597238 - Attachment is obsolete: true
Attachment #598746 - Flags: feedback?
Attachment #598746 - Flags: feedback? → feedback?(roc)
Comment on attachment 598746 [details] [diff] [review]
Makes GetVideoIntrinsicSize prefer video size over poster size

Review of attachment 598746 [details] [diff] [review]:
-----------------------------------------------------------------

This is very close...

::: layout/generic/nsVideoFrame.cpp
@@ +538,5 @@
>      return nsSize(nsPresContext::CSSPixelsToAppUnits(size.width), prefHeight);
>    }
>  
>    nsHTMLVideoElement* element = static_cast<nsHTMLVideoElement*>(GetContent());
> +  size = element->GetVideoSize(nsIntSize(0, 0));

I think you should probably pass -1,-1 here or something like that. A zero-sized video doesn't make much sense but probably is legal.
I'm pretty confident this patch should cover all the cases we have, and follow the spec at the same time.

It's likely going to break some reftests.. I should probably apply for level 1 commit access for the try server.
Attachment #598746 - Attachment is obsolete: true
Attachment #598746 - Flags: feedback?(roc)
Attachment #598978 - Flags: review?(roc)
Comment on attachment 598978 [details] [diff] [review]
Makes GetVideoIntrinsicSize prefer video size over poster size

Review of attachment 598978 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsVideoFrame.cpp
@@ +539,5 @@
>  
>    nsHTMLVideoElement* element = static_cast<nsHTMLVideoElement*>(GetContent());
> +  size = element->GetVideoSize(nsIntSize(-1, -1));
> +
> +  if (size.height == -1 && size.width == -1 && ShouldDisplayPoster()) {

You can just test size.height <= 0. No point in checking the width as well.

@@ +541,5 @@
> +  size = element->GetVideoSize(nsIntSize(-1, -1));
> +
> +  if (size.height == -1 && size.width == -1 && ShouldDisplayPoster()) {
> +    // Use the poster image frame's size.
> +    nsIFrame *child = mFrames.FirstChild();

This isn't quite right, we don't want to depend on the poster image frame being first in the list. Use mPosterImage->GetPrimaryFrame() instead.

@@ +543,5 @@
> +  if (size.height == -1 && size.width == -1 && ShouldDisplayPoster()) {
> +    // Use the poster image frame's size.
> +    nsIFrame *child = mFrames.FirstChild();
> +    if (child && child->GetType() == nsGkAtoms::imageFrame) {
> +      nsImageFrame* imageFrame = static_cast<nsImageFrame*>(child);

Use do_QueryFrame instead.
Attached patch Changes as per review (obsolete) — — Splinter Review 
I've made the changes you mentioned. I was curious as to why the poster was being assumed as the first child frame.
Attachment #598978 - Attachment is obsolete: true
Attachment #598978 - Flags: review?(roc)
Attachment #598987 - Flags: review?(roc)
Comment on attachment 598987 [details] [diff] [review]
Changes as per review

Review of attachment 598987 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsVideoFrame.cpp
@@ +543,5 @@
> +  if (size.height == -1 && ShouldDisplayPoster()) {
> +    // Use the poster image frame's size.
> +    nsIFrame *child = mPosterImage->GetPrimaryFrame();
> +    if (child && child->GetType() == nsGkAtoms::imageFrame) {
> +      nsImageFrame* imageFrame = do_QueryFrame(child);

With this, you don't need an explicit check of GetType(). If it's not an image frame, do_QueryFrame will return null.
Ok, cool. I'll make that change quickly and check it's all working still. I should probably write some mochi-tests for this shouldn't I?
A mochitest in content/media/tests is probably the way to go.

It's a bit tricky to test the case where there's no video frame displaying yet but we do have a poster image and we have determined the size of the video resource (we should be testing that the instrinsic size is the size of the resource, not the poster image). You could make the poster image a data URI so it loads really fast, hopefully faster than the video first frame, and of course a different size from the video, then just run a fast setTimeout loop until the video reaches the loadeddata state, constantly checking that the size of the <video> matches the size of the video resource if we're in a state >= HAVE_METADATA.
I'm not sure if these tests are perfect, but what they do is check that one video which preloads the video data will be 320x240 and that the other which has no preload, takes on the size of the poster ( 400x400 )
Attachment #598987 - Attachment is obsolete: true
Attachment #598987 - Flags: review?(roc)
Attachment #599030 - Flags: review?(roc)
darn, copy-pasta leftovers in the test case... D:
Can you make the poster image a solid black PNG, so that it compresses better? Those huge data URLs are a bit scary :-)
Comment on attachment 599030 [details] [diff] [review]
Fixes as per review, first run on tests for the bug

Review of attachment 599030 [details] [diff] [review]:
-----------------------------------------------------------------

One other thing is that we try to make our media tests backend-independent. Here it's enough to add a test for canPlayType("video/ogg") and if that returns false, do todo(false, "Ogg not supported") and finish immediately.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #16)
> Comment on attachment 599030 [details] [diff] [review]
> Fixes as per review, first run on tests for the bug
> 
> Review of attachment 599030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One other thing is that we try to make our media tests backend-independent.
> Here it's enough to add a test for canPlayType("video/ogg") and if that
> returns false, do todo(false, "Ogg not supported") and finish immediately.

Or call getPlayableVideo(), like we do in this test:
http://mxr.mozilla.org/mozilla-central/source/content/media/test/test_mixed_principals.html?force=1#27
I took a look at getPlayableVideo(), but it didn't seem like I could be sure what dimensions the video I received would be, so I opted instead for the canPlayType method. The poster image is now a grayscale black png, so it's _much_ smaller.
Attachment #599030 - Attachment is obsolete: true
Attachment #599030 - Flags: review?(roc)
Attachment #600258 - Flags: review?(roc)
Comment on attachment 600258 [details] [diff] [review]
Reduces poster image data URI size, adds check for video/ogg

Review of attachment 600258 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/test_bug726904.html
@@ +29,5 @@
> +var v1 = document.getElementById( "v1" ),
> +    v2 = document.getElementById( "v2" );
> +
> +if( !v1.canPlayType( "video/ogg" ) ) {
> +  todo(false, "Ogg not supported");

This isn't quite right because if Ogg is disabled you just go ahead and run the test anyway. You need to skip the rest of the test.
(In reply to Chris DeCairos (:cadecairos) from comment #18)
> Created attachment 600258 [details] [diff] [review]
> Reduces poster image data URI size, adds check for video/ogg
> 
> I took a look at getPlayableVideo(), but it didn't seem like I could be sure
> what dimensions the video I received would be, so I opted instead for the
> canPlayType method. The poster image is now a grayscale black png, so it's
> _much_ smaller.

If you add the dimensions of the video to the test entry in manifest.js (like we do here http://mxr.mozilla.org/mozilla-central/source/content/media/test/manifest.js#10) then you know the dimensions.

Then in your test, do the following:

var resource = getPlayableVideo(gSmallTests);
// And then the values are:
resource.name // the file name for the test
resource.width // the width
resource.height // the height

So you'd need to add the width and height for seek.webm to gSmallTests to do this (it's also 320x240 I think).
I've changed this around to use GetPlayableVideo, which will get either webm or ogv, or fail with a todo.
Attachment #600258 - Attachment is obsolete: true
Attachment #600258 - Flags: review?(roc)
Attachment #603154 - Flags: review?(roc)
Comment on attachment 603154 [details] [diff] [review]
Changes to use GetPlayableVideo in tests

Review of attachment 603154 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/test_bug726904.html
@@ +23,5 @@
> +    poster = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAZAAAAGQCAAAAACl1GkQAAAAAXNSR0IArs4c6QAAAAlwSFlzAAALEwAACxMBAJqcGAAAALJJREFUeNrtwQENAAAAwqD3T20ON6AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHg0cq4AATRk8BYAAAAASUVORK5CYII",
> +    resource = getPlayableVideo(gSeekTests);
> +
> +function v1_metaDataLoaded(){
> +  is( v1.videoWidth, 320, "Intrinsic width should be 320" );

Should that be:
is( v1.videoWidth, resource.width, "Intrinsic width should be match video dimensions");
?

And similar with videoHeight?

Not every resource returned by GetPlayableVideo() is guaranteed to be 320x240; that's why we have width/height stored in the manifest.
I pushed the patch to try, and unfortunately the reftests fail:
https://tbpl.mozilla.org/?tree=Try&rev=5d9d90851e42
Attached patch Modified reftests (obsolete) — — Splinter Review 
I've modified the ref tests so that they take into account the new behaviour that <video> elements have. also, modified the mochitests to use gSmallTests, which it should've been doing originally. Now I can get height/width.
Attachment #603154 - Attachment is obsolete: true
Attachment #603154 - Flags: review?(roc)
Attachment #603594 - Flags: review?(roc)
Comment on attachment 603594 [details] [diff] [review]
Modified reftests

Review of attachment 603594 [details] [diff] [review]:
-----------------------------------------------------------------

Over to cpearce to review the test changes from now on :-)
Attachment #603594 - Flags: review?(roc) → review?(cpearce)
Comment on attachment 603594 [details] [diff] [review]
Modified reftests

Review of attachment 603594 [details] [diff] [review]:
-----------------------------------------------------------------

I made a few comments, r=cpearce with them addressed.

We'd better land this and bug 517363 together, else we'll be rendering the poster image with incorrect aspect ratio while scaling, which would look weird. And for bug 517363 you'll need to update the tests again to reflect that the poster image should be scaled WRT the aspect ratio to fit inside the video element's intrinsic size.

::: content/media/test/test_bug726904.html
@@ +23,5 @@
> +    poster = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAZAAAAGQCAAAAACl1GkQAAAAAXNSR0IArs4c6QAAAAlwSFlzAAALEwAACxMBAJqcGAAAALJJREFUeNrtwQENAAAAwqD3T20ON6AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHg0cq4AATRk8BYAAAAASUVORK5CYII",
> +    resource = getPlayableVideo(gSmallTests);
> +
> +function v1_metaDataLoaded(){
> +  is( v1.videoWidth, resource.width, "Intrinsic width should be 320" );

Nit: "Intrinsic width should match video width". resource.width may not be 320. ;) Ditto for height.

::: layout/reftests/ogg-video/poster-1.html
@@ +1,4 @@
>  <!DOCTYPE HTML>
>  <html>
>  <body style="background:white;">
>  <!-- Test if poster frame displays correctly when poster is different size. -->

Change comment to: "Ensure video element displays at poster size when video's intrinsic size isn't available."

::: layout/reftests/ogg-video/poster-15.html
@@ +5,5 @@
>       them etc. -->
>  <body style="background:white;">
>  <video src="black140x100.ogv"
>         poster="green70x30.png"
> +       preload="none"

This test is supposed to test that backgrounds are drawn correctly when the poster is too small, but with preload="none" that won't be tested. For this patch, leave this as preload="none", but make sure you remove this preload="none" when you fix bug 517363, and adjust the test to ensure that backgrounds are properly rendered.

::: layout/reftests/ogg-video/poster-7.html
@@ +7,2 @@
>         id="v"
> +       onload="document.getElementById('v').poster = 'red140x100.png'; setTimeout(function(){document.documentElement.className = '';}, 0);"

When you land bug 517363, revert this to use red160x120.png, so we can test letter boxing etc. This means there's not much point in removing poster-ref-red160x120.html, since we'll need it again.

::: layout/reftests/ogg-video/poster-ref-blue140x100.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<body style="background:white;">
> +<img src="blue140x100.png" alt="poster">

You need to include blue140x100.png in this patch, or make it a datauri.

::: layout/reftests/ogg-video/poster-ref-red140x100.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<body style="background:white;">
> +<img src="red140x100.png" alt="poster">

You need to include red140x100.png in this patch, or make it a datauri.
Attachment #603594 - Flags: review?(cpearce) → review+
Comment on attachment 603594 [details] [diff] [review]
Modified reftests

Review of attachment 603594 [details] [diff] [review]:
-----------------------------------------------------------------

Some style nits:

::: content/media/test/test_bug726904.html
@@ +26,5 @@
> +function v1_metaDataLoaded(){
> +  is( v1.videoWidth, resource.width, "Intrinsic width should be 320" );
> +  is( v1.videoHeight, resource.height, "Intrinsic height should be 240" );
> +  is( v2.clientWidth, 400, "clientWidth should be 400" );
> +  is( v2.clientHeight, 400, "clientHeight should be 400" );  

'is(foo', not 'is( foo' (note space), 4 times.

@@ +30,5 @@
> +  is( v2.clientHeight, 400, "clientHeight should be 400" );  
> +  SimpleTest.finish();
> +}
> +
> +if (resource != null ) {

'if (resource)'

@@ +46,5 @@
> +  document.body.appendChild(v2);
> +  
> +} else {
> +
> +  todo(false, "No types supported");

Get rid of the empty lines above
Attached patch Changes to patch, as per review (obsolete) — — Splinter Review 
carrying forward. r=cpearce
Attachment #603594 - Attachment is obsolete: true
Attachment #607011 - Flags: review+
I fixed up all the reftests, and fixed a bug in my change to nsVideoFrame.cpp. here's the TBPL for this: https://tbpl.mozilla.org/?tree=Try&rev=699aec48de21

Now that I have this working I'll get Bug 517363 working on top of this patch, so that we can land them at the same time :)
Attachment #607011 - Attachment is obsolete: true
Attachment #615212 - Flags: review?(cpearce)
Comment on attachment 615212 [details] [diff] [review]
Fixes to reftests, minor bugfix to nsVideoFrame.cpp

Review of attachment 615212 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsVideoFrame.cpp
@@ +539,4 @@
>    }
>  
>    nsHTMLVideoElement* element = static_cast<nsHTMLVideoElement*>(GetContent());
> +  size = element->GetVideoSize(nsIntSize(-1, -1));

This code smells bad. Because you removed |size|'s initialization to (300,150), you now have to include the default width of 300 twice below. If someone changes one, they need to remember to the other. This makes the code code harder to maintain.

Also, since GetVideoSize() returns the default value on failure and overwriting size's initialized value, you need to add more conditional logic to handle its failure, complicating the code's logic.

So let's refactor to clean things up. Since GetVideoSize() isn't used anywhere else, lets change GetVideoSize() to return NS_ERROR_FAILURE when there's no video size, and pass in a reference to size which is filled upon success.

Then your "if (size.height==-1..." can become:
  if (NS_FAILED(element->GetVideoSize(size)) && ShouldDisplayPoster()) ...
... and you don't need to remove the default initialization, or repeat the default values, or add extra conditional logic either.
Attachment #615212 - Flags: review?(cpearce) → review-
Attached patch Changes to patch, as per review (obsolete) — — Splinter Review 
I've made the changes to GetVideoSize, and modified the two places it's used. Looks a lot better now too!

I've pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=29a15c65cf0a
Attachment #615212 - Attachment is obsolete: true
Attachment #616035 - Flags: review?(cpearce)
Comment on attachment 616035 [details] [diff] [review]
Changes to patch, as per review

Review of attachment 616035 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. Please remove the blank line I mention below, and re-upload the patch. No need to request review again, just mark it as r+ when you upload, and say something like "carrying forward r=cpearce". Thanks!

::: layout/generic/nsVideoFrame.cpp
@@ +540,5 @@
>      return nsSize(nsPresContext::CSSPixelsToAppUnits(size.width), prefHeight);
>    }
>  
>    nsHTMLVideoElement* element = static_cast<nsHTMLVideoElement*>(GetContent());
> +

Can you remove this blank line?
Attachment #616035 - Flags: review?(cpearce) → review+
Attached patch carrying forward r=cpearce (obsolete) — — Splinter Review 
Removed extra line.

cpearce: I guess we now need to get Bug 517363 good to go? I'm waiting on a review on that one from roc. The patch is pretty ugly though, it'll need some work to get it just right.
Attachment #616035 - Attachment is obsolete: true
Attachment #616450 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/559fdd1b5e07
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 754860
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: