449142 - Video is automatically scaled to fit inside its "box" while maintaining the aspect ratio.

Status: VERIFIED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Biju]

Attachment: video_size.html

If I specify a height and width video looses the aspect ratio
see video_size.html

But per Philip Jägenstedt and Ian Hickson at
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-August/015558.html

Video is automatically scaled to fit inside its "box" while maintaining the aspect ratio. 

# Video content should be rendered inside the element's playback area such 
# that the video content is shown centered in the playback area at the 
# largest possible size that fits completely within it, with the video 
# content's adjusted aspect ratio being preserved.
  -- http://www.whatwg.org/specs/web-apps/current-work/#video

Comment 1

[Biju]

There are situations where we need to fix this.

But if we fix this, whether there will a way when for web developer to alter the aspect ratio when really need it ?

I wish there was attribute like "fit" with following values

== "fit" attribute values ===
* center          - if it is small keep at center
* stretch         - stretch/skew height and width to fit
* stretch-height  - stretch/skew height only to fit,
                    adjust width to maintain aspect ratio.
* stretch-width   - stretch/skew width only to fit,
                    adjust height to maintain aspect ratio.
* stretch-best    - stretch/skew height or width to maximum
                    so that other will fit in
                    and maintain aspect ratio.

Comment 2

[Biju]

better test case 
http://simon.html5.org/test/html/semantics/video/003.htm

other test cases
http://www.whatwg.org/specs/web-apps/current-work/show-tests.html?section=video

Comment 3

[James May [:fowl2]]

Could there be a pref to force video to "center"? Some machines can't handle scaling video in real time at the same time.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

The code is simple. This is a good time to add some video reftests so I've done that. As well as testing this bug (the aspect-ratio tests), I've also added a few other tests that we really should have had already. Fortunately Ogg encodes a black PNG to perfect black; black140x100.ogv is one frame (duration 0.04s) of black.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One little problem with those tests is that they'll fail if Ogg is disabled. There doesn't seem to be a way to get reftests to skip an entire 'include' if MOZ_OGG is not 1 ... should there be? I don't really want to make each test conditional.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Note to self, once these land I should add a test for canvas.drawImage(video), since it would be trivial.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 353397 [details] [diff] [review]
fix

sr=dbaron

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> One little problem with those tests is that they'll fail if Ogg is disabled.
> There doesn't seem to be a way to get reftests to skip an entire 'include' if
> MOZ_OGG is not 1 ... should there be? I don't really want to make each test
> conditional.

If you want to fix this, probably the best way is include-if().  However, I'd really like to get rid of the autoconf dependency in reftest so that they can be packaged to run in separate builds, so I'd prefer to just assume that ogg is enabled (i.e., test our defaults).  (We'll also catch the regression of accidentally changing our default that way...)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 8ab5a111e00b to trunk

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backed out because the reftests failed on Windows and Linux

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The reftests failed on Windows and Linux because <video> needs to behave like images and use EXTEND_PAD to avoid sampling outside the video when we scale the video. Of course, we can't just do that, because X is broken (and cairo-quartz is broken in a lesser way).

So we need to introduce another hack to switch on the destination surface type and go to nearest-neighbour filtering for X etc. What I'd like to do is take the EXTEND_PAD_EDGE mode in gfxPattern, remove it, and replace it with a method SetExtendPadEdge(gfxASurface* aDestSurface) that sets EXTEND_PAD for all surface types except for Quartz, where it sets EXTEND_NONE, and X, where it sets EXTEND_NONE and also sets filtering to nearest-neighbour. The border-image code should be using this too. Note that the current code switches on the type of the source surface, which I think is incorrect; it's the destination surface that matters. (I assume if the types don't match then we fall back to extracting an image surface from the source and then converting that for the destination.)

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Adds SetExtendPadEdges as described in the previous comment ... but I decided to pass a context instead of the surface, since that's what callers have.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 353728 [details] [diff] [review]
fix v2

I can't believe we have to go through all this pain to work around the underlying brokenness :(

Comment 14

[Zack Weinberg (:zwol)]

I'm really confused about the special casing of Quartz here -- per the discussion in bug 468496, I have empirical evidence that EXTEND_PAD and EXTEND_NONE are *not* the same thing for cairo-quartz surfaces, and from code inspection, EXTEND_PAD does *not* appear to take a slow path there.

Again from that bug, EXTEND_NONE/nearest-neighbor is not an effective substitute for EXTEND_PAD on Linux -- you still can get edge artifacts.

And, from code inspection, it *is* the source surface that matters -- if you're rendering across a type mismatch, it looks to do the rendering on the source side, whatever that is, and then do a straight copy across.

If it were me I would be setting EXTEND_PAD everywhere and we would just accept the performance hit entailed.  (Then we could/should go fix Cairo and XRender, but it wouldn't be a correctness issue anymore.)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #14)
> I'm really confused about the special casing of Quartz here -- per the
> discussion in bug 468496, I have empirical evidence that EXTEND_PAD and
> EXTEND_NONE are *not* the same thing for cairo-quartz surfaces, and from code
> inspection, EXTEND_PAD does *not* appear to take a slow path there.

I'm just trying to be consistent with images. If we should change the handling of Quartz, file a bug about that and discuss it with Vlad --- I think he wrote that Quartz code.

> Again from that bug, EXTEND_NONE/nearest-neighbor is not an effective
> substitute for EXTEND_PAD on Linux -- you still can get edge artifacts.

OK. Again, I'm just trying to be consistent with images.

> And, from code inspection, it *is* the source surface that matters -- if you're
> rendering across a type mismatch, it looks to do the rendering on the source
> side, whatever that is, and then do a straight copy across.

You're right, so I'll revise the patch to fix that.

> If it were me I would be setting EXTEND_PAD everywhere and we would just accept
> the performance hit entailed.  (Then we could/should go fix Cairo and XRender,
> but it wouldn't be a correctness issue anymore.)

In general I don't think we can take that performance hit.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In this case here, the source is always an image surface, so if you're right about the source surface mattering (and I think you are, from my reading of the code, although it's hairy) I might just be able to forget about touching this workaround code and use EXTEND_PAD.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

This is basically v1 with EXTEND_PAD used for all platforms. Passes tests on my Linux VM. I'll rerun Mac tests and try relanding.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I have to say, though, that I'm deeply disturbed by the path that cairo takes here ... painting each frame of video as a gfxImageSurface requires an XGetImage call to get the current surface pixels, then a software composite, followed by XPutImage! That's a horrible way to implement it, it should just be pushing the RGBA data to the server and avoiding server round trips (or doing something even better like letting us push YUV data). Seems like we should have a gfx bug on fixing that, right?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mac tests still pass.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed cc039e5c24ce

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #18)
> I have to say, though, that I'm deeply disturbed by the path that cairo takes
> here ... painting each frame of video as a gfxImageSurface requires an
> XGetImage call to get the current surface pixels, then a software composite,
> followed by XPutImage! That's a horrible way to implement it, it should just be
> pushing the RGBA data to the server and avoiding server round trips (or doing
> something even better like letting us push YUV data). Seems like we should have
> a gfx bug on fixing that, right?

Note this only happens because of the EXTEND_PAD that's involved; if it's EXTEND_NONE or one of the others that are supported on the server, then a temporary xlib surface should be created by pattern_acquire_surface and the image data should be xputimage'd.  However, now that I think about it, I worry that we still might do a GetImage... hopefully we don't.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, it's not as bad as I thought. But still, it would be great if we could do some optimization in cairo so that at least when video is not scaled we're not doing an XGetImage for each frame of video!

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 3c6d0d5b182c to 1.9.1

Comment 24

[Tony Chung [:tchung]]

Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090224 Shiretoko/3.1b3pre Ubiquity/0.1.5
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090224 Minefield/3.2a1pre
and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; en-US; rv:1.9.1b3pre) Gecko/20090224 Shiretoko/3.1b3pre