Clean up the high-quality scaler

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
7 years ago
11 months ago

People

(Reporter: joe, Assigned: edransch)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++][mentor=nobody@mozilla.org][leave open])

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

7 years ago
Currently the high quality scaler imported as part of bug 486918 imports a bunch of unnecessary Chromium code. It should be cleaned up in various ways, including removing the extra copies of Chromium code (which will require fixing the build when not using them), using SSE support (if and when upstream Chromium turns it on too), and possibly switching it out for the pixman-using version that Jeff implemented.

The relevant files are gfx/2d/image_operations.cpp and gfx/2d/convolver.cpp.
Hi,

I am new to the OSS scene and would like to get started on this bug, could you please guide me to getting started?

Thanks.
Reporter

Comment 2

7 years ago
Hi Anthony!

The first thing I'd recommend you do is merge in the changes that the Chromium authors have made upstream to their image_operations.cc and image_operations.h. These files are located at http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/image_operations.cc?view=log and http://src.chromium.org/viewvc/chrome/trunk/src/skia/ext/image_operations.h?view=log. Their counterparts exist in Mozilla's code at gfx/2d/image_operations.cpp and .h.

Note that I had to make a bunch of changes to get their files compiling in Mozilla's code, so you can't easily just copy over their files, but instead you need to see what changes they've made and make analogous changes to our code.

If you don't yet have a build of Firefox, here's where to get started: https://developer.mozilla.org/En/Simple_Firefox_build

Comment 3

7 years ago
Hello,

I've found that no changes have been made to image_operations.h, convolver.h, or convolver.cc from upstream since Joe committed these files originally. There have been two changes made to image_operations.cc since then. The first was fixing a half-pixel misalignment; this is included in my patch. The other was updating references to stack_container.h because of a directory change. Since the directories in mozilla-central/ipc/chromium haven't changed, I didn't include it in the patch.

Joe, is there any documentation available that will help guide us in figuring out what parts of the code are relevant to Mozilla so that we can clean up these files? Also, please let me know if I've left anything out, or if something needs to be corrected. Thanks.
Attachment #692687 - Flags: review?(joe)
Reporter

Comment 4

7 years ago
Comment on attachment 692687 [details] [diff] [review]
Merge upstream changes to image_operations.cpp

Review of attachment 692687 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have any particular advice about this, unfortunately. In general the "clean up" work should probably eventually lead to us totally excising the code and using the pixman scaler that Jeff wrote; there are somewhat in-progress patches on bug 486918.
Attachment #692687 - Flags: review?(joe) → review+
Reporter

Comment 5

7 years ago
We should probably leave this open for now for future work, and just merge in the changes. Thanks Yazen!
Assignee: nobody → yghannam
Keywords: checkin-needed
Whiteboard: [lang=c++][mentor=joe@drew.ca] → [lang=c++][mentor=joe@drew.ca][leave open]
(Pushing this without a Try link against my better judgment...)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf51fc0485f0

Yazen, to make life easier for those committing on your behalf, please make sure that your future patches follow the directions below so that all the needed commit information is present in the patch. Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
Keywords: checkin-needed
Reporter

Comment 8

6 years ago
Our next step is probably improving Jeff's pixman lanczos filter, and landing it in upstream pixman, so we don't have to have this separate piece of infrastructure. Tim Terriberry had a bunch of review comments on things that need to be done: https://bugzilla.mozilla.org/show_bug.cgi?id=486918#c140
Hey there Yazen, are you still working on this bug? Joe, is it still in progress?
Flags: needinfo?(yghannam)

Comment 10

6 years ago
Hi Liz, I'm planning on working on it, following Joe's advice, once I finish this semester of school (in another two weeks). Somebody else can take it if it needs to be completed soon. Thanks.
Flags: needinfo?(yghannam)
Reporter

Comment 11

6 years ago
This doesn't need to be solved quickly, so please continue with your plans, Yazen! Thanks!

Comment 12

6 years ago
Hi everyone. It looks like I won't have time to work on this, so the Assignee should be reset. Thanks.
Assignee: yghannam → nobody
Assignee

Comment 13

6 years ago
These two commits to the pixman repository from December 2012 add some functionality implementing box and Lanczos filters.

http://cgit.freedesktop.org/pixman/commit/?id=6fd480b17c8398c217e4c11e826c82dbb8288006
http://cgit.freedesktop.org/pixman/commit/?id=6915f3e24f4169260a8ad6ab7ff3087388dbe5db

They have also included a demo which scales an image up and down using these filters.

http://cgit.freedesktop.org/pixman/tree/demos/scale.c

I would like to determine whether or not we can leverage this code for scaling. 
As someone with more familiarity with Pixman, do you have intuition as to whether we can use it?

If it seems like we can use it, I can write a patch so we can test it on a try build.
If not, we can go ahead with merging Jeff's filter implementations into the Pixman codebase.
Flags: needinfo?(joe)
Reporter

Comment 14

6 years ago
It definitely feels like we can use that, and the bonus is that it already exists and will be optimized for us as time goes on. I say go for it.
Flags: needinfo?(joe)
Assignee

Comment 15

6 years ago
Posted patch WIP patch for scaling (obsolete) — Splinter Review
This is a progress update and request for feedback.

I have a few questions to begin with and I'll add more comments on specific lines below.

1) Are there automated tests that verify image scaling? I'd like to run some tests for correctness across a variety of images. 

2) I would also like to test performance of the patch in comparison to the Skia implementation. Do you have any suggestions on how to measure the performance impact?
Assignee: nobody → edransch.contact
Attachment #783915 - Flags: feedback?(joe)
Assignee

Comment 16

6 years ago
Comment on attachment 783915 [details] [diff] [review]
WIP patch for scaling

Review of attachment 783915 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Scale.cpp
@@ +28,5 @@
> +    double fScale_x, fScale_y;
> +    int numSamples = 4;
> +    pixman_fixed_t *params;
> +    // TODO: set this variable based on format argument
> +    pixman_format_code_t pixmanFormat = (format == FORMAT_B8G8R8A8) ? PIXMAN_a8r8g8b8 : PIXMAN_x8r8g8b8;

I only considered two formats here. I think it's probably worth writing a function to convert between these formats.

Do you have a suggestion for which hpp file I could put that function in?

@@ +61,5 @@
> +        PIXMAN_KERNEL_LANCZOS3,
> +        PIXMAN_KERNEL_LANCZOS3,
> +        PIXMAN_KERNEL_LANCZOS3,
> +        numSamples,
> +        numSamples);

I arbitrarily chose both the numSamples value and the kernel type here. 

Perhaps someone with more familiarity with the lanczos filter can comment here to suggest the best choice. 

Otherwise, we can experiment to find a balance of quality and performance.
Reporter

Comment 17

6 years ago
(In reply to Erick Dransch [:edransch] from comment #15)
> 1) Are there automated tests that verify image scaling? I'd like to run some
> tests for correctness across a variety of images. 

Not high-quality image scaling, no. I would gladly help to create such a test suite.
 
> 2) I would also like to test performance of the patch in comparison to the
> Skia implementation. Do you have any suggestions on how to measure the
> performance impact?

You could do it in an invasive way, by adding telemetry for time spent downscaling images. I'd gladly r+ that.
Reporter

Comment 18

6 years ago
Comment on attachment 783915 [details] [diff] [review]
WIP patch for scaling

Review of attachment 783915 [details] [diff] [review]:
-----------------------------------------------------------------

This broadly looks great. 

A style nit: be sure to line up parameters to a function vertically with the open bracket (e.g.

Foo(a,
    b);

I'm asking Jeff for feedback on the values of the kernel and the number of samples here specifically.

::: gfx/2d/Scale.cpp
@@ +15,5 @@
>  
> +
> +pixman_fixed_t double_to_fixed(double x){
> +    return x * 65536.0 + 0.5;
> +}

Is there a reason you didn't use pixman_double_to_fixed() here?

@@ +24,5 @@
>             SurfaceFormat format)
>  {
> +    int numParams;
> +    double invertScale_x, invertScale_y;
> +    double fScale_x, fScale_y;

I suggest not using fScale as a name :)

@@ +28,5 @@
> +    double fScale_x, fScale_y;
> +    int numSamples = 4;
> +    pixman_fixed_t *params;
> +    // TODO: set this variable based on format argument
> +    pixman_format_code_t pixmanFormat = (format == FORMAT_B8G8R8A8) ? PIXMAN_a8r8g8b8 : PIXMAN_x8r8g8b8;

I think it'd be reasonable to put that in 2d/Tools.h (or 2D/Helpers.h, but that file is empty).

@@ +61,5 @@
> +        PIXMAN_KERNEL_LANCZOS3,
> +        PIXMAN_KERNEL_LANCZOS3,
> +        PIXMAN_KERNEL_LANCZOS3,
> +        numSamples,
> +        numSamples);

If this was in imagelib, I'd say we want numsamples to be a pref so it could be adjusted more easily, but a #define would probably be better than just a stack value in the function.
Attachment #783915 - Flags: feedback?(joe)
Attachment #783915 - Flags: feedback?(jmuizelaar)
Attachment #783915 - Flags: feedback+
Assignee

Comment 19

6 years ago
Posted patch use pixman scaling (obsolete) — Splinter Review
I've addressed Joe's comments regarding this patch. 

If you have suggestions on how to determine the best parameters for the filter (to balance performance and quality), I would appreciate it.

I can add a separate patch to add Telemetry data for time spent downscaling. Would that be useful data to collect?
Attachment #783915 - Attachment is obsolete: true
Attachment #783915 - Flags: feedback?(jmuizelaar)
Attachment #803055 - Flags: feedback?(jmuizelaar)
Comment on attachment 803055 [details] [diff] [review]
use pixman scaling

Review of attachment 803055 [details] [diff] [review]:
-----------------------------------------------------------------

We should get some basic performance information. I would suggest just adding logging using TimeStamp and the dst and src image sizes and seeing how these compare.

::: gfx/2d/Scale.cpp
@@ +8,5 @@
>  
> +#include <stdlib.h>
> +#include <algorithm>
> +
> +#include <iostream>

Is this needed?

@@ +41,5 @@
> +                                                         (uint32_t *) dstData, dstStride ? dstStride : 4);
> +
> +    pixman_f_transform_init_identity(&ftransform);
> +    pixman_f_transform_scale(&ftransform, NULL, scale_x, scale_y);
> +    pixman_f_transform_invert(&ftransform, &ftransform);

Can we just construct the inverted transform instead of inverting afterwards? i.e. using invertScale? and perhaps we can construct invertScale from src/dst to avoid needless inaccuracy
Attachment #803055 - Flags: feedback?(jmuizelaar) → feedback+
Assignee

Comment 21

6 years ago
I used http://autoindustry.files.wordpress.com/2009/03/ferrari_dino_concept_car_by_nixseraph.jpg and some crude logging to check the performance difference between pixman and Skia.

On my machine, Skia requires 50-90ms to scale the image, depending on the size of the dest image (smaller is faster).
Pixman requires 500-800ms, depending on dest size (smaller is faster).

I played with a few different configurations for the pixman filters, but I didn't find any that provided high quality and comparable speed to Skia.

I will try to gather some more fine-grained performance information to determine why the performance difference is so large.
If any possible explanations jump to mind, I would appreciate any hints.
Attachment #803055 - Attachment is obsolete: true
Attachment #808403 - Flags: feedback?(jmuizelaar)
Assignee

Comment 22

6 years ago
This is the logging code I added to provide some basic information about the performance difference between Skia and pixman
Assignee

Comment 23

6 years ago
I've played around with performance sampling and the Cleopatra tool, which shows that both implementations spend most of their time in the convolution loops, as expected.
There are a few differences in implementation between Pixman and Skia. I've highlighted the ones that I think might have a big impact:

1) The Pixman implementation uses both a sampling and a reconstruction filter. If I'm interpreting correctly, the Skia implementation uses only the reconstruction filter, not the sampling filter.
We can get the same behaviour as the Skia implementation if we set the sampling filter in Pixman to Box.
If we change the sampling filter in Pixman to Box, we do improve performance, but it's still about 3-4x slower than the Skia implementation.

2) The Skia implementation calculates the filter values around each vertical and horizontal pixels ahead of time, and stores them. While rescaling, it just looks up in the array to get the filter values.
The Pixman implementation calculates a generic filter, and calculates the filter matrix for each specific point as it goes.

3) The Skia implementation runs horizontally on 4 rows at a time, and stores the result in a ring buffer, runs the vertical filters for each pixel in the 4 rows, and repeats.
The Pixman implementation runs the full filter (both horizontal and vertical) for each pixel.
Both 2) and 3) might have a major effect on performance. It's probably a combination of these that causes the performance difference.

I'm not sure what the appropriate next step is here. Am I correct in assuming that a performance difference of 3x means we should stay with the Skia implementation?
We could re-evaluate in the future if new optimizations are added to Pixman.

Any other thoughts?
Flags: needinfo?(jmuizelaar)

Updated

6 years ago
See Also: → 856793
(In reply to Erick Dransch [:edransch] from comment #23)
> I'm not sure what the appropriate next step is here. Am I correct in
> assuming that a performance difference of 3x means we should stay with the
> Skia implementation?
> We could re-evaluate in the future if new optimizations are added to Pixman.
> 
> Any other thoughts?

Yeah, it sounds like we should stay with current Skia implementation at least until the performance is improved.
Flags: needinfo?(jmuizelaar)

Updated

5 years ago
Whiteboard: [lang=c++][mentor=joe@drew.ca][leave open] → [lang=c++][mentor=nobody@mozilla.org][leave open]
From a quick read-through of this bug, it sounds like there's nothing else that needs to be done here. Since a patch landed (comment 7) should we just close out this bug?
Flags: needinfo?(edransch.contact)
Assignee

Comment 26

5 years ago
I have no objections to closing the bug. 

If we decide to re-launch the investigation about replacing Skia with Pixman for high quality scaling, we can file a new bug for that.
Assignee

Updated

5 years ago
Flags: needinfo?(edransch.contact)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Fabulous!

Joe, you mentioned possibly writing automated tests for this in Comment 17. Are there any tests for this?

Also, I'll set the target milestone for this to Nightly (Firefox 32) for now.
Flags: in-testsuite?
Target Milestone: --- → mozilla32
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.