Closed Bug 1111945 Opened 9 years ago Closed 9 years ago

Try to re-implement image editing with plain JS instead of WebGL

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: djf, Assigned: djf)

References

Details

Attachments

(1 file)

When the gallery app was first implemented, I used WebGL to get really smooth editing when the user dragged the exposure compensation slider. Since then, WebGL has proved to be more troble than it is worth, and I'm filing this bug to replace it with plain JavaScript (probably running in a worker)

Preliminary investigation shows that dragging the exposure compensation slider with the current WebGL implementation gives a very smooth 50 frames per second.  Re-implementing that in pure JS (without workers) slows us down to 25 frames per second. Actually performing the processing on the 270x360 image takes 15ms, which seems fast enough (a 2700x3600 image would take 1.5s, which will be faster than we can do with WebGL for full-size images). Hopefully I can speed things up by using a worker, but even if I can't, 25fps for the exposure slider is probably acceptable, given all the other benefits that will come from getting rid of webgl.
Work in progress is in this branch: https://github.com/davidflanagan/gaia/tree/gallery-remove-webgl
Assignee: nobody → dflanagan
I've updated my WIP patch to do the image processing in a worker and because I'm able to pass an ArrayBuffer back and forth without copying I'm able to maintain a frame rate in the high 20s when moving the exposure slider back and forth and I think there is better responsiveness using a separate thread.
It is not the full 50fps I got with webgl, but I'm still hopeful that this will be a better approach.
David, just to know, what are the benefits or getting rid of webGL?
(In reply to Fabrice Desré [:fabrice] from comment #3)
> David, just to know, what are the benefits or getting rid of webGL?

WebGL is blazingly fast for editing an on-screen preview of an image, but the shaders are brutally hard to write, maintain and extend and we've got no real WebGL expertise on the Media apps team.  And it turns out that WebGL is really not very good at all for editing full-size images that are much, much larger than the screen. The time we spend copying image tiles to and from the GPU seems to outweigh the performance advantage we get by using the GPU in the first place.  

Also, with WebGL, and since about v1.4 we've also had to deal with contextlost events, which make the code really complex.

So the benefits of getting rid of WebGL are:

- less code
- code that is easier to maintain and extend
- code in only one language
- the potential for future optimizations with asm.js

My preliminary work seems to indicate that performance will still be quite acceptable without WebGL, and we might actually get a speed up in the time it takes to edit fullsize images. (Though I haven't tested that yet).
My work in progress patch is now usable: https://github.com/mozilla-b2g/gaia/pull/26866

Editing the full-size image takes 3 to 4 seconds which is just about the same (or maybe faster) than what the webgl version takes.
TODO:

I think this patch breaks cropping, so I'll have to fix that.

I also want to merge the existing histogram computation worker into the new image_worker.js 

While I'm doing that, I will experiment with modifying the auto enhancement algorithm to do full histogram equalization using the cumulative histogram as the curve rather than doing a linear fit like we do now. There is a good explanation here: http://www.generation5.org/content/2004/histogramEqualization.asp

I should also modify the preview images in the effects ui so that I'm generating the thumbnails with image_processor.js instead of using CSS filters, since those don't match precisely and require duplication of the matrix.
Amy: sorry we didn't get to work on the gallery edit effects at the Portland workweek. If you'd like to suggest new effects, this would be a good bug to use for fixing them. Just let me know the matrices you'd like me to try and a test image to try them on.
Flags: needinfo?(amlee)
(In reply to David Flanagan [:djf] from comment #7)
> Amy: sorry we didn't get to work on the gallery edit effects at the Portland
> workweek. If you'd like to suggest new effects, this would be a good bug to
> use for fixing them. Just let me know the matrices you'd like me to try and
> a test image to try them on.

Hi David, 

I was looking at the original matrices that Patryk provided in comment 2 in this bug https://bugzilla.mozilla.org/show_bug.cgi?id=799998. Can you provide me some context of what these numbers are based on?

Thanks
Flags: needinfo?(amlee) → needinfo?(dflanagan)
Amy: the matrices listed in bug 799998 are like those described in this Flash tutorial: http://code.tutsplus.com/tutorials/manipulate-visual-effects-with-the-colormatrixfilter-and-convolutionfilter--active-3221

It is a fairly standard technical way of describing color effects, and I thought it was a Photoshop thing that you'd be familiar with. But googling for it it appears to not be used in Photoshop.  SVG <feColorMatrix> filters are also based on the same kind of matrix, if that helps at all.
Flags: needinfo?(dflanagan)
(In reply to David Flanagan [:djf] from comment #9)
> Amy: the matrices listed in bug 799998 are like those described in this
> Flash tutorial:
> http://code.tutsplus.com/tutorials/manipulate-visual-effects-with-the-
> colormatrixfilter-and-convolutionfilter--active-3221
> 
> It is a fairly standard technical way of describing color effects, and I
> thought it was a Photoshop thing that you'd be familiar with. But googling
> for it it appears to not be used in Photoshop.  SVG <feColorMatrix> filters
> are also based on the same kind of matrix, if that helps at all.

Hi David, 

I did some research into the color matrix and there doesn't seem to be any translatable measurement between Photoshop or Lightroom into the matrices you're using.

What are the current matrices we have for the filters? When I was reviewing the bug that Patryk filed (Bug 799998) he had posted the following matrices:

Fashion High Contrast... "Blue Steel"
1, .25, .65, 0,
.1, 1, .65,  0,
.1, .25, 1, .1,
0, 0, 0, 1

Faded Photo
1, .2, .2, .03,
.2, .7, .2, .05, 
.1, 0, .8, 0,
0, 0, 0, 1

Black & White
.65, .25, .10, 0,
.65, .25, .10, 0,
.65, .25, .10, 0,
0, 0, 0, 1

Sepia
0.393, 0.769, 0.189, 0,
0.349, 0.686, 0.168, 0,
0.272, 0.534, 0.131, 0,
0, 0, 0, 1 

In that bug, it seemed that the results on the device did not match up with his image examples. Would there be a difference in the appearance of the filters now that we switched to JS?  
--------------------------------------------------------------------------------------
Also, 

Take a look at the examples here at the bottom of the page:
http://camanjs.com/examples

Vintage (Sepia)
Pinhole (Black/White)
Hazy Days (Fade)
Cross Process (Blue Steel)

Can we use these examples as a baseline for our filters if Patryk's original matrices can't be translated accurately? 

Thanks
Flags: needinfo?(dflanagan)
I've fixed cropping and re-implemented auto enhancement to do histogram equalization. I'm think I might be getting worse results than the more basic linear equalization that we had before, however, so I might switch that back.
Flags: needinfo?(dflanagan)
I just saw bug 1102712 and discovered that for more than a year we've been ignoring devicePixelRatio and displaying edit previews using CSS pixels instead of device pixels, resulting in lower quality images on devices like the Flame. 

I've fixed this in bug 1102712, and with webgl there is hardly any performance difference on the Flame, even though there are not 2.25 times as many pixels. I'm worried, however, that in this bug there will be a noticeable difference in performance, and that the reponsiveness of the exposure slider will be affected.
Depends on: 1102712
> 
> In that bug, it seemed that the results on the device did not match up with
> his image examples. Would there be a difference in the appearance of the
> filters now that we switched to JS?  

The app is using the matricies that Patryk specified. My recollection is also that he was unsatisfied with the appearance of the results. I don't know if that is still an issue, however. When I've got more time, I'll edit his test image from that bug with the filters on the phone (both with and without webgl) to see whether the result looks like the result images he provided.

The reason I was asking about this now is that I was under the impression that we wanted different effects than those that we had now. 

> 
> Take a look at the examples here at the bottom of the page:
> http://camanjs.com/examples
> 
> Vintage (Sepia)
> Pinhole (Black/White)
> Hazy Days (Fade)
> Cross Process (Blue Steel)
> 
> Can we use these examples as a baseline for our filters if Patryk's original
> matrices can't be translated accurately? 
> 

Those are cool! I've looked at the code and they are not defined in terms of matrices but instead as a series of simpler image operations. I'm not certain that all of the component operations are linear and so these filters may not be representable as a matrix.
(In reply to David Flanagan [:djf] from comment #13)
> > 
> > In that bug, it seemed that the results on the device did not match up with
> > his image examples. Would there be a difference in the appearance of the
> > filters now that we switched to JS?  
> 
> The app is using the matricies that Patryk specified. My recollection is
> also that he was unsatisfied with the appearance of the results. I don't
> know if that is still an issue, however. When I've got more time, I'll edit
> his test image from that bug with the filters on the phone (both with and
> without webgl) to see whether the result looks like the result images he
> provided.
> 
> The reason I was asking about this now is that I was under the impression
> that we wanted different effects than those that we had now. 
> 
> > 
> > Take a look at the examples here at the bottom of the page:
> > http://camanjs.com/examples
> > 
> > Vintage (Sepia)
> > Pinhole (Black/White)
> > Hazy Days (Fade)
> > Cross Process (Blue Steel)
> > 
> > Can we use these examples as a baseline for our filters if Patryk's original
> > matrices can't be translated accurately? 
> > 
> 
> Those are cool! I've looked at the code and they are not defined in terms of
> matrices but instead as a series of simpler image operations. I'm not
> certain that all of the component operations are linear and so these filters
> may not be representable as a matrix.

Hi David, 

We want to improve on the quality of the filters since the resulting images on the device currently don't represent what Patryk's sample images show. If this means we have to change the effects then I'm open to it but I think Sepia and Black and White are pretty standard and we should still keep those. The alternative is that if we can't improve on the quality of the filters then we would remove them from the gallery app.
I've merged the patch from bug 1102712 into this bug, so that I'm now doing pure JS editing of a larger preview image using device pixels rather than CSS pixels. Performance has dropped and it takes ~100ms or so to process each exposure change. This may still be acceptable. I've modified the slider so that the thumb follows the user's finger responsively even if image editing can't keep up, so the frame rate is decent, but the edits sometimes lag the slider.

If the performance is not acceptable, I'd suggest that we revise the UX to replace the slider with buttons for increasing and decreasing exposure in quarter step increments. The slider is the only UX that promises real-time updates. For everything else our response time is fine.
I'm going to try to get back to this bug. All the discussion of adding new effects or changing the current effects are really out of scope here. For now, this bug is just about replacing the webgl code with pure JS code.

I've figured out some possible performance improvements:

1) in the image processing loop, if I use a plain Uint8Array instead of the default Uint8ClampedArray, I can get a decent speedup.  If all the pixel values are pre-computed (in clamped arrays themselves) then I don't have to worry about underflow and overflow and there is no need for the clamping. This should be a trivial change.

2) For the file saving case, it ought to be possible to break the image processing across two or three workers and reduce the total time to save an image on multi-core devices.  This shouldn't be too hard, but it may involve doing some explicit copying of bytes that was not required before.

3) Since I last worked on this I have figured out how to hand-write asm.js code, and I think it ought to be possible to implement the core image processor editing function as an asm.js module. If so, that will probably be another substantial speed boost, and might bring the exposure slider frame rate back up to be competitive with webgl.
I've done #1 and #2 above and am surprised that they don't seem to have much effect. Going from one thread to two does reduce image processing time, but only by a little bit, not anywhere close to the 50% I would have expected.  Perhaps the time is dominated by the message passing time required for communication between the main thread and the workers? That would surprise me, though, since I'm using transferrable ArrayBuffers that are not supposed to require copying.
Also, I'm seeing bug 1119157 intermittently in my tests. Image editing fails sometimes because of that bug. And when it does, the image editor gets into a broken state.  It is a 2.1+ blocker, though, so hopefully will be fixed soon.
Depends on: 1119157
Converting to asm.js gives a nice speed improvement. It looks like the actual pixel manipulation is now twice as fast. And with the overhead for passing typed arrays back and forth, we end up with things being about 30% faster overall.
The code in https://github.com/mozilla-b2g/gaia/pull/26866/files is almost ready for review. 

I want to check again on my nexus 5 to see whether varying the number of workers gets us a speedup or not, because it was pretty strange that I wasn't seeing faster results when using multiple threads.  Other than that, though, I think this is just about done.
Punam,

This is the patch I was describing to you. It is long, but a lot of that is tests and comments.  Don't feel like you need to understand all of what the asm.js code is doing.
Attachment #8579165 - Flags: review?(pdahiya)
Comment on attachment 8579165 [details] [review]
link to patch on github

Hi David

The patch looks good and simplifies the underlying image editing code a lot. I have noted few comments/question on github.  The main reason I am marking patch r- is because of the bug seen while re-editing an image saved after cropping with aspect ratio 3:2

STR
1. Open image taken using camera in gallery
2. Edit the image - a) by cropping to aspect ratio 3:2, click apply and save to save edited image
3. Click edit icon in footer to edit the saved edited image
4. Crop by selecting aspect ratio 2:3 or 3:2 and click apply.
5. Select exposure/effects in edit tools and try to change exposure/effects, image processing fails with below error in logs

03-19 08:40:11.421 E/Gallery ( 1253): Content JS ERROR: TypeError: invalid arguments processImage@app://gallery.gaiamobile.org/js/image_processor.js:174:22
03-19 08:40:11.421 E/Gallery ( 1253): self.onmessage@app://gallery.gaiamobile.org/js/image_processor_worker.js:22:18
03-19 08:40:11.421 E/Gallery ( 1253):  
03-19 08:40:11.421 E/Gallery ( 1253):     at self.onmessage (app://gallery.gaiamobile.org/js/image_processor_worker.js:30:4)

The bug is seen with saved 3:2 edited image and not while editing the original image.

Thanks
Attachment #8579165 - Flags: review?(pdahiya) → review-
Comment on attachment 8579165 [details] [review]
link to patch on github

Punam,

I've addressed your review comments and fixed the bug you found.

The bug occurred when you crop an image to an aspect ratio that fits better on the screen. This creates a new preview that is larger than the old one, and the ImageProcessor object can't handle the new, larger preview.  There was code that was supposed to catch this case and create a new processor but it was failing because the image_processor_thread.js class was not remembering its maximum image size value.

Please review again.
Attachment #8579165 - Flags: review- → review?(pdahiya)
Comment on attachment 8579165 [details] [review]
link to patch on github

Hi David
Thanks for updated rebased patch, it fixes the bug in #comment 22. I have tested and it looks good. I see a crop overlay bug which shows when phone is rotated in crop edit tool. Sorry for missing it in last review.

STR:
1. Click Edit image
2. Click crop edit tool and select aspect ratio say 2:3
3. Rotate the phone

Crop Overlay should be shown as per new crop region. I debugged and the issue is the code to show crop overlay inside resize()  [Line 897 - 906] is getting executed before edit completes (Line 893).

Thanks
Attachment #8579165 - Flags: review?(pdahiya) → review-
Comment on attachment 8579165 [details] [review]
link to patch on github

I'm glad that you're reviewing this patch, Punam! Thank you for finding and debugging this. I've updated the PR with a fix.
Attachment #8579165 - Flags: review- → review?(pdahiya)
Comment on attachment 8579165 [details] [review]
link to patch on github

Hi David,

Thanks for updated patch, it looks good to land and has my r+.
Attachment #8579165 - Flags: review?(pdahiya) → review+
Thanks, Punam.

Landed to master: https://github.com/mozilla-b2g/gaia/commit/3be46b117d16d9422de9acd230ab2223d5f30c52
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: