Closed Bug 577843 Opened 9 years ago Closed 9 years ago

Video scaling (up or down) is too slow

Categories

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

x86
Windows XP
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla2.0b5
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: alvaro.segura, Assigned: cajbir)

References

()

Details

Attachments

(1 file, 8 obsolete files)

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: nobody → chris.double
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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)
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)
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.
(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.
(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.
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?
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)
Blocks: 583138
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+
Change 'bool' usage to 'PRBool' as per review comment
Attachment #461406 - Attachment is obsolete: true
Attachment #461971 - Flags: review+
Attached patch Rolled up and rebased patch (obsolete) — Splinter Review
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Depends on: 589718
Depends on: 589717
Backed out:
http://hg.mozilla.org/mozilla-central/rev/cfb5b914e2b4
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Fixes for build breakage (obsolete) — Splinter Review
Fixes for the build breakage identified in bug 589717 and bug 589718
Attachment #468246 - Attachment is obsolete: true
Attachment #468546 - Flags: review?
Attachment #468546 - Flags: review? → review?(roc)
Whiteboard: [needs landing]
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.
Review carried forward.
Attachment #466875 - Attachment is obsolete: true
Attachment #468546 - Attachment is obsolete: true
Attachment #485965 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/8ecd9dc6684e
Whiteboard: [needs landing]
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Depends on: 618458
Blocks: 639778
You need to log in before you can comment on or make changes to this bug.