Closed Bug 934787 Opened 11 years ago Closed 10 years ago

Tracking: Low-hanging fruit in Gallery's usage of WebGL

Categories

(Firefox OS Graveyard :: Gaia::Gallery, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bjacob, Assigned: djf)

References

Details

Attachments

(3 files)

Gallery's usage of WebGL is not always optimally efficient; and while optimizing without profiling is generally a bad idea, here we're talking to a large extent about GPU-side work that wouldn't show up on a general profiler, and anyway there is some low-hanging fruit that should be easy enough to pick.

Will file individual bugs blocking this one.
Depends on: 934790
Depends on: 934796
Depends on: 934810
Depends on: 934813
On my hamachi / master, while dragging the exposure slider, I currently get frame intervals around 60 ms, i.e. around 16 FPS.
No longer depends on: 934810
Depends on: 935353
Looking at the ImageEditor's code I was wondering if we are doing too much work for setting up the geometry and mapping the texture on it. Since we are only talking about a simple rectangle, shouldn't the texture just be mapped to 0..1? Anyway, I am not really an expert on all the low level WebGL stuff. 

Do you mind to take a look at https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/ImageEditor.js#L1454 ? I was surprised why we had so many input for the shader? Why do the dimensions of the canvas matter for the shader? Especially looking at the gl_position calculation made me suspicious. 
Maybe this could be simplified.
(In reply to Tom Herold from comment #2)
> Looking at the ImageEditor's code I was wondering if we are doing too much
> work for setting up the geometry and mapping the texture on it. Since we are
> only talking about a simple rectangle, shouldn't the texture just be mapped
> to 0..1? Anyway, I am not really an expert on all the low level WebGL stuff. 

See the patch bug 934813, it simplifies this.

> 
> Do you mind to take a look at
> https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/ImageEditor.
> js#L1454 ? I was surprised why we had so many input for the shader? Why do
> the dimensions of the canvas matter for the shader? Especially looking at
> the gl_position calculation made me suspicious. 
> Maybe this could be simplified.

I didn't pay much attention to that because that's not going to be performance-critical:
 - vertex shader performance rarely matters vs. fragment shader performance --- especially not here as we're only drawing 1 quad.
 - having more inputs than necessary isn't the main problem we had here in the fragment shader, rather our main problem here in the fragment shader was that we were doing more math than necessary.

But you're right, there is definitely room for further simplification even after my patches.
(In reply to Benoit Jacob [:bjacob] from comment #3)
> (In reply to Tom Herold from comment #2)
> > Looking at the ImageEditor's code I was wondering if we are doing too much
> > work for setting up the geometry and mapping the texture on it. Since we are
> > only talking about a simple rectangle, shouldn't the texture just be mapped
> > to 0..1? Anyway, I am not really an expert on all the low level WebGL stuff. 
> 
> See the patch bug 934813, it simplifies this.
> 
> > 
> > Do you mind to take a look at
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/ImageEditor.
> > js#L1454 ? I was surprised why we had so many input for the shader? Why do
> > the dimensions of the canvas matter for the shader? Especially looking at
> > the gl_position calculation made me suspicious. 
> > Maybe this could be simplified.
> 
> I didn't pay much attention to that because that's not going to be
> performance-critical:
>  - vertex shader performance rarely matters vs. fragment shader performance
> --- especially not here as we're only drawing 1 quad.
>  - having more inputs than necessary isn't the main problem we had here in
> the fragment shader, rather our main problem here in the fragment shader was
> that we were doing more math than necessary.
> 
> But you're right, there is definitely room for further simplification even
> after my patches.

You are right. This not super performance critical. I was aiming more for code simplicity and less JS work here.
Hey :djf, please consider merging my patches in B2G 1.3, if you agree with them. They apply in this order: bug 934790, bug 934813, bug 935353.
Flags: needinfo?(dflanagan)
When applying the patches in this order bug 934790, bug 934813, bug 935353. The last one (935353) breaks the edit mode of the gallery. I get the following message in console:

E/GeckoConsole(  779): [JavaScript Error: "Error: Error compiling fragment shader:ERROR: 0:13: 'rgb_min_max_values' : undeclared identifier
E/GeckoConsole(  779): ERROR: 0:13: 'rgb_min_max_values' :  left of '[' is not of type array, matrix, or vector
E/GeckoConsole(  779): ERROR: 0:13: '=' :  cannot convert from 'float' to '-component vector of float'
E/GeckoConsole(  779): ERROR: 0:14: 'rgb_min_max_values' :  left of '[' is not of type array, matrix, or vector
E/GeckoConsole(  779): ERROR: 0:14: '=' :  cannot convert from 'float' to '-component vector of float'
Flags: needinfo?(bjacob)
Maybe something went wrong in the last rebasing, let me check.
I've uploaded a properly rebased patch to bug 935353. Tested locally. Sorry about the bad previous patch.
Flags: needinfo?(bjacob)
Diego, how does the new patch on bug 935353 look?
Flags: needinfo?(dmarcos)
Looking good now! Thanks!
Without the patch, when I slide the exposure slider as fast as I can, I get a max framerate of about 28.

With the patch, I get a max framerate in the low thirties, so the webgl changes make a noticeable difference.

Note that the gallery code does not redraw the image unless the exposure slider moves more than 0.25 (on a scale from -3 to 3). So sliding from left to right would only create 24 frames.  This might be limiting the frame rate. Maybe we can get a better rate if I redraw the image more often, now that the code is more efficient.
Flags: needinfo?(dflanagan)
I've modified the patch to allow 96 distinct "stops" for exposure instead of the previous 24.  Moving my finger back and forth as fast as I can still gives me a framerate in the low 30s.  But swiping one way as fast as I can gets framerates as high as 55, so I think we're good here.
Could you have the preview updates drawn in requestAnimationFrame callbacks? That is "the right way" to drive any animation, should give the smoothest animation, and automatically avoids drawing faster than the compositor can handle (thes points depend on the browser engine's implementation, but that is how things are supposed to be done, at least). That would also give the simplest code.
These are Benoit's WebGL patches, with a few Gaia tweaks.

Diego, I only need you to review the changes around line 214 and the changes involving the needsUpload property and argument.
Attachment #8344980 - Flags: review?(dmarcos)
Fabrice: I'd like approval to land this as soon as Diego has reviewed so we can get it into 1.3
Flags: needinfo?(fabrice)
r+. Just suggested a change to make the code easier to understand
Flags: needinfo?(dmarcos)
Attachment #8344980 - Flags: review?(dmarcos) → review+
Just for fun I wanted to see some numbers. I attached a patch that continuously repaints the image using requestAnimationFrame. (repainting is not dependent on the user dragging the slider)

FPS printed in console. adb logcat | grep FPS

With none of the optimizations applied and without user interaction (I don't touch the screen)

Stable 53 FPS

After applying 934790: 

Stable FPS 60

At least ~10% gain. Not bad. It might be more. My understanding is that requestAnimationFrame is going to limit your repaints to a maximum of 60 FPS.
Attached patch fps.patchSplinter Review
(In reply to David Flanagan [:djf] from comment #16)
> Fabrice: I'd like approval to land this as soon as Diego has reviewed so we
> can get it into 1.3

Looks good to me, please land.
Flags: needinfo?(fabrice)
\o/ !

Yes, requestAnimationFrame limits callbacks to 60 FPS.

Basically, for active tabs/applications, requestAnimationFrame will try to give you optimal callback times i.e. once per physical frame which is 60 Hz on most current devices; and (the following doesn't matter in the present case, but just FWIW) will try to properly schedule these callbacks so that animations based on timestamps obtained from rAF callbacks are optimally smooth. IOW, properly used rAF is 'the right way' to do animations.
Landed to master: https://github.com/mozilla-b2g/gaia/commit/01b4fe3a462dd40cb9e34bc659bd6c2446893b34
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee: nobody → dflanagan
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: