Video scaling (up or down) is too slow

RESOLVED FIXED in mozilla2.0b5

Status

()

Core
Audio/Video
--
minor
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Alvaro, Assigned: cajbir)

Tracking

unspecified
mozilla2.0b5
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(URL)

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Assignee: nobody → chris.double
(Assignee)

Updated

8 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 1

8 years ago
Created attachment 458938 [details] [diff] [review]
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.
Attachment #458938 - Flags: review?(roc)
(Assignee)

Comment 2

8 years ago
Created attachment 458942 [details] [diff] [review]
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.
Attachment #458942 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Duplicate of this bug: 495854
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 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.
(Assignee)

Comment 6

8 years ago
(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.
(Assignee)

Comment 7

8 years ago
(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.
(Assignee)

Comment 8

8 years ago
Created attachment 460389 [details] [diff] [review]
Patch to scale videos at YCbCr conversion time

Addresses review comments
Attachment #458942 - Attachment is obsolete: true
Attachment #460389 - Flags: review?(roc)
Attachment #458942 - Flags: review?(roc)
(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?
(Assignee)

Comment 10

8 years ago
Created attachment 461406 [details] [diff] [review]
Patch to scale videos at YCbCr conversion time

Removes size check and always performs scale as per review comment.
Attachment #460389 - Attachment is obsolete: true
Attachment #461406 - Flags: review?(roc)
Attachment #460389 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Blocks: 583138
(Assignee)

Comment 11

8 years ago
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 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
Attachment #461406 - Flags: review?(roc) → review+
BTW we should make an effort to get our improvements to the Chromium code back upstream.
blocking2.0: --- → betaN+
(Assignee)

Comment 14

8 years ago
Created attachment 461971 [details] [diff] [review]
Patch to scale videos at YCbCr conversion time

Change 'bool' usage to 'PRBool' as per review comment
Attachment #461406 - Attachment is obsolete: true
Attachment #461971 - Flags: review+
(Assignee)

Comment 15

8 years ago
Created attachment 466875 [details] [diff] [review]
Rolled up and rebased patch
Attachment #458938 - Attachment is obsolete: true
Attachment #461971 - Attachment is obsolete: true
Attachment #466875 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/dbbb9575aae1
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
(Assignee)

Updated

8 years ago
Depends on: 589718
(Assignee)

Updated

8 years ago
Depends on: 589717
Backed out:
http://hg.mozilla.org/mozilla-central/rev/cfb5b914e2b4
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

8 years ago
Created attachment 468246 [details] [diff] [review]
fix for the build issue identified in bug 589717
(Assignee)

Comment 19

8 years ago
Created attachment 468546 [details] [diff] [review]
Fixes for build breakage

Fixes for the build breakage identified in bug 589717 and bug 589718
Attachment #468246 - Attachment is obsolete: true
Attachment #468546 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #468546 - Flags: review? → review?(roc)
Whiteboard: [needs landing]
(Assignee)

Comment 20

8 years ago
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.
(Assignee)

Comment 21

7 years ago
Created attachment 485965 [details] [diff] [review]
Rolled up and rebased patch with commit comment

Review carried forward.
Attachment #466875 - Attachment is obsolete: true
Attachment #468546 - Attachment is obsolete: true
Attachment #485965 - Flags: review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Comment 22

7 years ago
http://hg.mozilla.org/mozilla-central/rev/8ecd9dc6684e
Whiteboard: [needs landing]
(Assignee)

Updated

7 years ago
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago7 years ago
Resolution: --- → FIXED
Blocks: 514848
Depends on: 618458

Updated

7 years ago
Blocks: 639778
You need to log in before you can comment on or make changes to this bug.