Implement Asynchronous image encoding for Canvas.toBlob

Implement Asynchronous image encoding for Canvas.toBlob

Main thread creates a new thread to carry out the computation-intensive asynchronous image encoding
operation. Afterwards the encoded image is passed back to the main thread to create blob and invoke user
callback.

Empty blob is passed to user callback in the senarios when the canvas has zero pixels or when the image encoding fails.

This code review issue is migrated to https://codereview.chromium.org/1355333005/.
BUG=523414
BUG=67587

[xlai (Olivia), 2015-09-09 23:50:48]

xlai@chromium.org changed reviewers:
+ junov@chromium.org

[xlai (Olivia), 2015-09-10 14:13:26]

Hi Justin,

Perhaps you want to take a look at the code first. 

I did try using the new idle scheduling framework before this but realized that
that framework cannot be suitable in this case. The scheduler has defined a
limited time for one single idle period and when the idle period ends, whatever
operation that's in progress needs to stopped as well. If the image encoding
cannot be finished before an idle period, I am not quite sure of the procedure
of saving the partial encoded image so that in the next idle period I can
continue whatever left behind.

Right now both audio and image data are encoded/decoded in separate threads. If
we want to change, then I would say both audio and image data codec processes
should be changed together for consistency.

[Justin Novosad, 2015-09-10 14:53:20]

As discussed by e-mail, the idea behind the idle task approach was to spread the
encoding of large images across multiple idle tasks by splitting the work into
scanlines or tiles.
But I think that can be achieved incrementally. For this first installment, you
could just use an idle task that does not respect the deadline (evil, but just
temporary), and in follow-up patches you can refactor the encoder integrations
to make them support partial encoding.

[Justin Novosad, 2015-09-10 15:04:17]

https://codereview.chromium.org/1331443009/diff/1/Source/core/html/HTMLCanvas...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1331443009/diff/1/Source/core/html/HTMLCanvas...
Source/core/html/HTMLCanvasElement.cpp:596: // Add a ref to keep image data
alive untio completion of encoding
"until"

https://codereview.chromium.org/1331443009/diff/1/Source/core/html/HTMLCanvas...
Source/core/html/HTMLCanvasElement.cpp:597: RefPtr<DOMUint8ClampedArray>
imageDataRef(imageData->data());
Isn't this incompatible with the use of ScopedDisposal above.  I think you would
want encodeImageAsync to be in charge of the disposal in this case.

https://codereview.chromium.org/1331443009/diff/1/Source/core/html/HTMLCanvas...
Source/core/html/HTMLCanvasElement.cpp:599: OwnPtr<WebThread> m_thread =
adoptPtr(Platform::current()->createThread("Async toBlob"));
no "m_" on local variables.  Spinning up a new thread for each call seems
inefficient.  Also, how come this works considering that m_thread goes out of
scope at the end of this function?

[xlai (Olivia), 2015-09-17 21:12:11]

Changed based on first code review (replies are inline).

I pass the quality argument by value from toBlob to encodeImageAsync. Somehow,
passing the double* pointer from main thread to another thread causes
unpredictable problems; moreover, the overhead of passing a double number by
value is very minimal. So it's better not to pass that by reference.

As an effect of this change, I've also replaced the "const double* quality"
parameter all over the toDataURL codes to "const double& quality". In fact, this
is in a better coding style as well.

https://codereview.chromium.org/1331443009/diff/1/Source/core/html/HTMLCanvas...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1331443009/diff/1/Source/core/html/HTMLCanvas...
Source/core/html/HTMLCanvasElement.cpp:597: RefPtr<DOMUint8ClampedArray>
imageDataRef(imageData->data());
On 2015/09/10 15:04:17, Justin Novosad wrote:
> Isn't this incompatible with the use of ScopedDisposal above.  I think you
would
> want encodeImageAsync to be in charge of the disposal in this case.

ImageData object itself can be disposed. I only need to have the
imageData->size() and imageData->data() for the m_thread.

https://codereview.chromium.org/1331443009/diff/1/Source/core/html/HTMLCanvas...
Source/core/html/HTMLCanvasElement.cpp:599: OwnPtr<WebThread> m_thread =
adoptPtr(Platform::current()->createThread("Async toBlob"));
On 2015/09/10 15:04:17, Justin Novosad wrote:
> no "m_" on local variables.  Spinning up a new thread for each call seems
> inefficient.  Also, how come this works considering that m_thread goes out of
> scope at the end of this function?


Done. Moved m_thread to private member of HTMLCanvasElement. Also create it once
only.

[Justin Novosad, 2015-09-21 14:12:38]

https://codereview.chromium.org/1331443009/diff/40001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1331443009/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:533: double quality = -1.0;
Use a named constant instead of a literal here. This shou8ld clarify what -1.0
means.

https://codereview.chromium.org/1331443009/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:545: // Create an OwnPtr for this worker
thread to hold the encoded image vector
"worker" is a loaded word, don't use it here. Also, comments should not be used
to echo the code, they should be used to explain things that are not obvious
from reading the code.

https://codereview.chromium.org/1331443009/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:548: // Perform image encoding
Useless comment

https://codereview.chromium.org/1331443009/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:552: // Pass the pointer of encoded image
vector to the main thread
Useless comment.

https://codereview.chromium.org/1331443009/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:579: double quality = -1.0;
use a constant

[xlai (Olivia), 2015-09-21 16:01:12]

Changed code accordingly.

[Justin Novosad, 2015-09-21 16:51:56]

https://codereview.chromium.org/1331443009/diff/80001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1331443009/diff/80001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:551: if (!ImageDataBuffer(imageSize,
imageData->data()).encodeImage(mimeType, quality, encodedImage.get())) {
Do we know that all of the encoders are thread safe? It would be a good idea to
add a test that verifies that each of the encoders can be invoked concurrently
on the main thread and on the "Async toBlob" thread.  To do this, you should
create a test the fires off a series of toBlob calls, then do a series of calls
toDataURL, wait for the toBlob calls to finish. Validate that all the blobs and
dataURLs are correct.  If there are any race conditions in the encoder code,
this test will flake, and TSAN is likely to catch it. You should have a version
of the test for each of the supported codecs.

https://codereview.chromium.org/1331443009/diff/80001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:598: m_thread =
adoptPtr(Platform::current()->createThread("Async toBlob"));
These threads are created on a per canvas basis, which is not a great. Ideally
we should have a managed pool of async task threads (topic to be brought up at
scheduler summit), but for now, we could just implement this in a way that
shares a single "Async toBlob" thread between all the canvases, to prevent a
thread explosion.  I could easily imagine a case where toBlob is called on a
temporary canvas element, repatedly which would tie thread clean-up to the
v8heap garbage collection, which could easily get out of hand.

[xlai (Olivia), 2015-09-22 22:03:08]

Added 3 layout tests to test the thread-safety of PNG, JPEG, WEBP image encoder
respectively. This is done by firing 9 toBlob calls and 2 toDataURL calls on
async thread and main thread respectively so that they race for the image
encoder function.

Also use s_thread as a static variable in HTMLCanvasElement.cpp. Added an
integer counter to track the number of Canvas instances. Once the number of
instances goes to zero, if the s_thread is not null, it'll be cleared promptly,
leaving no memory leak.

[xlai (Olivia), 2015-09-22 22:03:36]

Patchset #6 (id:100001) has been deleted

[Justin Novosad, 2015-09-23 19:38:57]

https://codereview.chromium.org/1331443009/diff/160001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-toDataURL-race-imageEncoder-jpeg.html
(right):

https://codereview.chromium.org/1331443009/diff/160001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-toDataURL-race-imageEncoder-jpeg.html:9:
var numToBlobCalls = 9; // number of toBlob calls on the original canvas
Comment does not add any useful information

https://codereview.chromium.org/1331443009/diff/160001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-toDataURL-race-imageEncoder-jpeg.html:10:
var numToDataURLCalls = 3; // number of toDataURL calls
same here

https://codereview.chromium.org/1331443009/diff/160001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-toDataURL-race-imageEncoder-jpeg.html:23:
shouldBeNonZero('canvasCtxs[0].getImageData(0, 0, 2500, 1750).data[25]');
why only context 0? data[25] is just the green component of a pixel, which seems
kind of arbitrary.

https://codereview.chromium.org/1331443009/diff/160001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-toDataURL-race-imageEncoder-jpeg.html:29:
shouldBe('canvasCtxs[' + i + '].getImageData(0, 0, 2500, 1750).data[25]',
'canvasCtxs[' + (i+1) + '].getImageData(0, 0, 2500, 1750).data[25]');
Comparing just one value in the image is not very robust. If there is
corruption, it could be in just a small region, which this test might not hit.
You should validate the entire image, but not with millions of calls to
ShouldBe, which would be inefficient. Instead, you can compare in JS and store
the result in a boolean, then call shouldBe on that boolean.
Also, try to re-write this in a way that avoids calling getImageData twice on
the same canvas. We do care a little bit about test run time.

https://codereview.chromium.org/1331443009/diff/160001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-toDataURL-race-imageEncoder-jpeg.html:34:
function onCanvasDrawCompleted(ctx_test, i)
'i' is unused

https://codereview.chromium.org/1331443009/diff/160001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-toDataURL-race-imageEncoder-png.html
(right):

https://codereview.chromium.org/1331443009/diff/160001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-toDataURL-race-imageEncoder-png.html:4: if
(window.testRunner) {
Most of the code in this file is identical to the jpeg flavor, you should factor
out the test code into a separate js file that can be called by the three
different tests, and use an argument to specify the mime type to be used for
encoding.

[Justin Novosad, 2015-09-23 20:32:32]

https://codereview.chromium.org/1331443009/diff/160001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1331443009/diff/160001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:84: const int InvalidQualityValue = -1.0;
I would call this Undefined instead of Invalid. "Invalid" suggest this is an
error, "Undefined" suggest a default should be used.

https://codereview.chromium.org/1331443009/diff/160001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:107: static WebThread* s_thread = 0;
You should make these statics members of HTMLCanvasElement.  Also, the name is
not very descriptive. s_blobEncoderThread would be better.

https://codereview.chromium.org/1331443009/diff/160001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:108: static int numCanvasInstances = 0;
missing s_

https://codereview.chromium.org/1331443009/diff/160001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:134: s_thread = 0;
This is a memory leak. You should make s_thread an OwnPtr<WebThread>, and call
clear() on it.

https://codereview.chromium.org/1331443009/diff/160001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:598: // ImageData object will be disposed
on leaving stack scope; only imageData->data and imageData->size are passed to
s_thread
This comment describes poorly what is happening. The scoped disposal forces the
imageData to unref it's data, which we keep alive for the thread by taking a
refptr. This way, the life time of the data is tied to the completion of the
async encode, rather than to the garbage collection of imageData.

https://codereview.chromium.org/1331443009/diff/160001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:607:
s_thread->taskRunner()->postTask(FROM_HERE, new
Task(threadSafeBind(&HTMLCanvasElement::encodeImageAsync,
AllowCrossThreadAccess(this),
AllowCrossThreadAccess(imageDataRef.release().leakRef()), imageData->size(),
AllowCrossThreadAccess(callback), encodingMimeType, quality)));
This is unsafe. We cannot guarantee that 'this' will stay alive long enough. 
You should add a layout test that does this:
1) create an offscreen canvas with: var canvas =
document.createElement('canvas');
2) draw to the canvas and call toBlob
3) dereference the canvas (set the variable to null)
4) force an immediate garbage collection (there is a test API for this either on
internals or testRunner)
5) wait for the callback to be invoked and verify the blob for correctness.