Closed Bug 449142 Opened 12 years ago Closed 11 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: BijuMailList, Assigned: roc)

Details

(Keywords: testcase, verified1.9.1)

Attachments

(2 files, 2 obsolete files)

Attached file 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
Component: DOM: Core & HTML → Video/Audio
QA Contact: general → video.audio
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.
Keywords: testcase
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Could there be a pref to force video to "center"? Some machines can't handle scaling video in real time at the same time.
Attached patch fix (obsolete) — Splinter Review
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.
Assignee: nobody → roc
Attachment #353397 - Flags: superreview?(dbaron)
Attachment #353397 - Flags: review?(chris.double)
Whiteboard: [needs review]
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.
Note to self, once these land I should add a test for canvas.drawImage(video), since it would be trivial.
Attachment #353397 - Flags: review?(chris.double) → review+
Comment on attachment 353397 [details] [diff] [review]
fix

sr=dbaron
Attachment #353397 - Flags: superreview?(dbaron) → superreview+
(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...)
Whiteboard: [needs review] → [needs landing]
Pushed 8ab5a111e00b to trunk
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Backed out because the reftests failed on Windows and Linux
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Whiteboard: [needs 191 landing] → [needs new patch]
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.)
Attached patch fix v2 (obsolete) — Splinter Review
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.
Attachment #353397 - Attachment is obsolete: true
Attachment #353728 - Flags: review?(vladimir)
Whiteboard: [needs new patch] → [needs review]
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 :(
Attachment #353728 - Flags: review?(vladimir) → review+
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.)
(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.
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.
Whiteboard: [needs review]
Attached patch fix v3Splinter Review
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.
Attachment #353728 - Attachment is obsolete: true
Attachment #354547 - Flags: superreview+
Attachment #354547 - Flags: review+
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?
Mac tests still pass.
Whiteboard: [needs landing]
Pushed cc039e5c24ce
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
(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.
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!
Pushed 3c6d0d5b182c to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.