577843 - Video scaling (up or down) is too slow

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alvaro]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1 ( .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1 ( .NET CLR 3.5.30729)

This is especially noticeable in slower machines such as netbooks, and/or very large videos.

Videos play smoothly when displayed in their original size (1:1 pixel scale), but scaling up or down makes playback choppy. In a netbook in power saving mode that makes the video support quite unusable in many cases. Interestingly, video decoding itself (Theora especially or WebM without resizing) even in HD performs well in such machines.

This bug report is not related to full screen video, which has recently been reimplemented with hardware acceleration and works smoothly, with less CPU than in-window scaling. Well, currently full screen crashes in my netbook [it's a known bug] so I'm not so sure.

Unlike images, it is common to have videos scaled to different sizes. (see http://jilion.com/sublime/video or youtube for example) so this is not a rare situation.

For reference, Chrome does this scaling faster and videos still play smooth when scaled. Maybe some code can be borrowed. If a better algorithm can't be implemented I would even fall back to a fast and simple point-sampled resizing (no interpolation or antialiasing) if the CPU can't keep up. I'd rather have smoothness with less quality than choppiness in a netbook.

Reproducible: Always

Steps to Reproduce:
1. Play the video in the above URL
2. zoom one step up or one step down
3. Use a netbook for a more noticeable effect
Actual Results:  
choppy playback in other than original size.

Expected Results:  
smooth playback.

Comment 1

[cajbir (:cajbir)]

Attachment: Add back Chromium YCbCr scaling code

This adds back the scaling code from the Chromium YCbCr conversion routines that we removed when originally adding the YCbCr code.

Comment 2

[cajbir (:cajbir)]

Attachment: Patch to scale videos at YCbCr conversion time

Add the ability to scale videos at YCbCr to RGB conversion time to improve performance.

This is done by setting a size hint when the video frame knows the size. The YCbCr image uses this hint to scale at the same time as conversion.

For testing the impact on performance I used the framecount patch in bug 580531 to compare framerates with and without this code added. On my machine it amounts to about a 20% improvement in framerate in a debug build on a test video.

Comment 4

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

Comment on attachment 458938 [details] [diff] [review]
Add back Chromium YCbCr scaling code

Shouldn't we just be removing remove_scale.patch, instead of applying remove_scale.patch and then applying add_scale.patch?

Comment 5

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

Comment on attachment 458942 [details] [diff] [review]
Patch to scale videos at YCbCr conversion time

Seems to me SetScaleHint should acquire a lock. Since only the subclass has a lock, I recommend moving msScaleHint into BasicImageContainer. In fact SetScaleHint should just go there since it's only relevant for BasicLayers.

BasicImageContainer::CreateImage should take the lock to get the scale hint.

+  bool prescale = scaleWidth > 0.0 && scaleHeight > 0.0;

PRBool

I think the scale hints should be gfxIntSize.

+  if (prescale && (width != aData.mPicSize.width || height != aData.mPicSize.height)) {

You don't need to check prescale here. Also, you could check gfxIntSize(width, height) != aData.mPicSize.

+ if (frameSize.width != static_cast<PRInt32>(r.Width())) {

I think you'd need to check height as well as width, but actually I don't think you need to check either. Just do the Scale unconditionally.

Comment 6

[cajbir (:cajbir)]

(In reply to comment #4)
> Shouldn't we just be removing remove_scale.patch, instead of applying
> remove_scale.patch and then applying add_scale.patch?

Unfortunately (or fortunately) a whole bunch of contributions have happened for other platforms and they sit as patches on top of the remove_scale patch. I can't test these so did it this way to avoid touching them.

At some point we will update to the latest Chromium code and I thought I'd re-do the patch arrangement (or find some other way of easing the maintenance) then.

Comment 7

[cajbir (:cajbir)]

(In reply to comment #5)
> I think you'd need to check height as well as width, but actually I don't think
> you need to check either. Just do the Scale unconditionally.

We can't do the scale unconditionally otherwise it scales the already pre-scaled image.

Comment 8

[cajbir (:cajbir)]

Attachment: Patch to scale videos at YCbCr conversion time

Addresses review comments

Comment 9

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

(In reply to comment #7)
> (In reply to comment #5)
> > I think you'd need to check height as well as width, but actually I don't think
> > you need to check either. Just do the Scale unconditionally.
> 
> We can't do the scale unconditionally otherwise it scales the already
> pre-scaled image.

But if frameSize == scaleHint then we're just passing 1,1 to transform.Scale, right?

Comment 10

[cajbir (:cajbir)]

Attachment: Patch to scale videos at YCbCr conversion time

Removes size check and always performs scale as per review comment.

Comment 11

[cajbir (:cajbir)]

Further to comment 4, comment 6 and discussion's in person with Robert I've raised bug 583138 to move towards to newer Chromium YCbCr conversion and scaling code. At that time we'll fix the patch ordering mentioned in comment 4.

Comment 12

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

Comment on attachment 461406 [details] [diff] [review]
Patch to scale videos at YCbCr conversion time

+  bool prescale = scaleWidth > 0.0 && scaleHeight > 0.0;

You still need to fix this to PRBool

Comment 13

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

BTW we should make an effort to get our improvements to the Chromium code back upstream.

Comment 14

[cajbir (:cajbir)]

Attachment: Patch to scale videos at YCbCr conversion time

Change 'bool' usage to 'PRBool' as per review comment

Comment 15

[cajbir (:cajbir)]

Attachment: Rolled up and rebased patch

Comment 16

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/dbbb9575aae1

Comment 17

[Karl Tomlinson (:karlt)]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/cfb5b914e2b4

Comment 18

[cajbir (:cajbir)]

Attachment: fix for the build issue identified in bug 589717

Comment 19

[cajbir (:cajbir)]

Attachment: Fixes for build breakage

Fixes for the build breakage identified in bug 589717 and bug 589718

Comment 20

[cajbir (:cajbir)]

Commenting on the [needs landing] whiteboard added. This needs to be landed alongside bug 583138 as it degrades video quality too much. Bug 583138 fixes that.

Comment 21

[cajbir (:cajbir)]

Attachment: Rolled up and rebased patch with commit comment

Review carried forward.

Comment 22

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/8ecd9dc6684e