Markus Stange [:mstange] Reporter
	Description • 12 years ago

<canvas>.toBlob image encoding happens on the main thread at the moment.
It shouldn't.

Luckily the API is already asynchronous.

	Markus Stange [:mstange] Reporter
	Comment 1 • 12 years ago

Here's a rough patch that seems to work mostly, though that might be due to sheer luck.
With this patch applied, the latest patch in bug 678392 performs much better.

	Markus Stange [:mstange] Reporter
	Comment 2 • 12 years ago

forgot to qrefresh

	Stephen A Pohl [:spohl] Assignee
	Comment 3 • 12 years ago

This patch works really well with the work in bug 678392 indeed! I'm going to keep this patch applied while working on bug 678392. Thanks Markus!

	Stephen A Pohl [:spohl] Assignee
	Comment 4 • 12 years ago

I noticed that after about 5 minutes of use, the Firefox process had more than 400 threads running. The normal number (without the patch here) seems to be closer to 30. My case is probably extreme since I have the patch for bug 678392 applied as well and <canvas>.toBlob() is called for each snapshot, but I thought it was important enough to point out.

	Markus Stange [:mstange] Reporter
	Comment 5 • 12 years ago

OK, that's what I suspected... looks like I'm leaking every single thread I create.

	Markus Stange [:mstange] Reporter
	Comment 6 • 12 years ago

Here's a better patch that correctly shuts down the threads it creates. It also doesn't copy the encoded stream unnecessarily and it doesn't break toDataURL.
Still needs lots of cleanup.

	Markus Stange [:mstange] Reporter
	Comment 7 • 12 years ago

lots of cleanup, moved the implementation to imgTools.cpp, still some work left

	Stephen A Pohl [:spohl] Assignee
	Comment 8 • 12 years ago

(In reply to Markus Stange from comment #7)
> Created attachment 689898 [details] [diff] [review]
> wip
> 
> lots of cleanup, moved the implementation to imgTools.cpp, still some work
> left

Debug builds seem to struggle with this latest patch. Optimized passes:

../../dist/include/nsCOMPtr.h:1458:38: error: comparison of distinct pointer types ('const PRThread *' and 'const nsIThread *')
    return const_cast<const U*>(lhs) == static_cast<const T*>(rhs.get());
           ~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../image/src/imgTools.cpp:486:43: note: in instantiation of function template specialization 'operator==<nsIThread, PRThread>' requested here
  NS_ABORT_IF_FALSE(PR_GetCurrentThread() == mWorkerThread, "wrong thread");
                                          ^
../../dist/include/nsDebug.h:45:11: note: expanded from macro 'NS_ABORT_IF_FALSE'
    if (!(_expr)) {                                           \
          ^
1 error generated.
make[6]: *** [imgTools.o] Error 1
make[5]: *** [src_libs] Error 2
make[4]: *** [libs_tier_platform] Error 2
make[3]: *** [tier_platform] Error 2
make[2]: *** [default] Error 2
make[1]: *** [realbuild] Error 2
make: *** [build] Error 2
program finished with exit code 2

https://tbpl.mozilla.org/php/getParsedLog.php?id=17732491&tree=Try

try job:
https://tbpl.mozilla.org/?tree=Try&rev=87683cddc69b

	Markus Stange [:mstange] Reporter
	Comment 9 • 12 years ago

fixed

	Markus Stange [:mstange] Reporter
	Comment 10 • 12 years ago

I think this is ready for a first review. Kyle, do you want to do the review?

The patch consists of three parts: The first part adds the EncodeImageData API to imgITools and implements it with off-main-thread encoding, the second part converts CanvasRenderingContext2D.cpp and WebGLContext.cpp to use this API, and the third part makes toBlob and mozFetchAsStream take advantage of the asynchronicity.

	Markus Stange [:mstange] Reporter
	Comment 11 • 12 years ago

How important is it to keep the number of threads under control? At the moment I'm not checking anything and just launching a new thread for every call to toBlob / mozFetchAsStream, so any webpage could launch an arbitrary number of (short-lived) threads.

	Stephen A Pohl [:spohl] Assignee
	Comment 12 • 12 years ago

Updated for current trunk, no other changes. Carrying over r? flag.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 13 • 12 years ago

Thanks for writing this patch Stephen.  Unfortunately it may be a while until I get to review it :-/  I'm going to try to give some comments later this week but other stuff might come up.

	Stephen A Pohl [:spohl] Assignee
	Comment 14 • 12 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #13)
> Thanks for writing this patch Stephen.  Unfortunately it may be a while
> until I get to review it :-/  I'm going to try to give some comments later
> this week but other stuff might come up.

This patch was actually written by Markus like so many other things, not me. :-)

I'm particularly interested in this fix for bug 678392. Ideally, we would get to check in this patch before bug 678392, but the review is pending there as well.

	Stephen A Pohl [:spohl] Assignee
	Comment 15 • 11 years ago

Updated for current trunk. Carrying r?

	Josiah Bruner [:jsbruner]
	Comment 16 • 11 years ago

This patch is broken on the latest trunk.

	Stephen A Pohl [:spohl] Assignee
	Comment 17 • 11 years ago

Updated for current trunk.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 18 • 11 years ago

Comment on attachment 703364 [details] [diff] [review]
v1

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

I don't understand why a lot of this complexity is here.  All the image encoders are already asynchronous.  You can create one, hand it out as an nsI[Async]InputStream (because imgIEncoder inherits from that) and pump data into it and do the encoding from a background thread.  I especially don't understand why we need to implement Read and ReadSegments with blocking rather than just returning NS_BASE_STREAM_WOULD_BLOCK.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 19 • 11 years ago

Also creating one thread per call makes this far too easy to DOS from content.

	Markus Stange [:mstange] Reporter
	Comment 20 • 11 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #18)
>  I especially don't
> understand why we need to implement Read and ReadSegments with blocking
> rather than just returning NS_BASE_STREAM_WOULD_BLOCK.

Because that would break toDataURL and mozGetAsFile; they both require the stream to act in a blocking way.

	Stephen A Pohl [:spohl] Assignee
	Comment 21 • 11 years ago

There seem to be a few test failures on try with this patch. Most of them seem to be due to:

###!!! ABORT: wrong thread: 'NS_GetCurrentThread() == mWorkerThread', file ../../../image/src/imgTools.cpp, line 483

See https://tbpl.mozilla.org/?tree=Try&rev=c34a0aa57611 for a try run with only this patch applied.

	Stephen A Pohl [:spohl] Assignee
	Comment 22 • 11 years ago

The abort mentioned in comment 21 is due to mWorkerThread being null. A try run with some debug info resulted in the following:

****************************** 0x143ac4530 NS_GetCurrentThread()
****************************** 0x143ac4530 mWorkerThread

****************************** 0x13ed32b50 NS_GetCurrentThread()
****************************** 0x13ed32b50 mWorkerThread

****************************** 0x148fbba20 NS_GetCurrentThread()
****************************** 0x0         mWorkerThread
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Parent 425] ###!!! ABORT: wrong thread: 'NS_GetCurrentThread() == mWorkerThread', file ../../../image/src/imgTools.cpp, line 486

The log is available here:
https://tbpl.mozilla.org/php/getParsedLog.php?id=19716344&tree=Try

Not sure yet why mWorkerThread is null in some situations.

	Josiah Bruner [:jsbruner]
	Comment 23 • 11 years ago

FYI, this bug is broken on the current trunk.

	Stephen A Pohl [:spohl] Assignee
	Comment 24 • 11 years ago

Updated for current trunk.

This patch also fixes the test failures observed on try. These were due to a race condition: The AsyncImageEncoder constructor used to create new threads with an initial ImageEncodingEvent event. The event could complete before the pointer to the thread was assigned to mWorkerThread. When this occurred, NS_ABORT_IF_FALSE(NS_GetCurrentThread() == mWorkerThread, "wrong thread") would be false in AsyncImageEncoder::EncodingFinished, since mWorkerThread was still 0. In optimized builds, this could cause crashes due to null-dereferences of mWorkerThread in AsyncImageEncoder::ShutdownWorkerThread(). The fix was to create the threads with a nullptr as initial event and dispatching the ImageEncodingEvent after the pointer to the thread was assigned to mWorkerThread.

To compare the before and after, here two try runs:
https://tbpl.mozilla.org/?tree=Try&rev=c34a0aa57611
https://tbpl.mozilla.org/?tree=Try&rev=a5e60e710739

We'd like to land this patch before landing the patches for bug 678392 because it eliminates two major performance regressions in Talos.

	Stephen A Pohl [:spohl] Assignee
	Comment 25 • 11 years ago

Comment on attachment 714399 [details] [diff] [review]
v1.1

Robert, would you have time to review this patch?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 26 • 11 years ago

Yes.

	Markus Stange [:mstange] Reporter
	Comment 27 • 11 years ago

Nice find, Stephen!

I remembered something else about why I did what I did in the patch.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #18)
> I don't understand why a lot of this complexity is here.  All the image
> encoders are already asynchronous.  You can create one, hand it out as an
> nsI[Async]InputStream (because imgIEncoder inherits from that) and pump data
> into it and do the encoding from a background thread.

If I hand out the imgIEncoder as the nsIAsyncInputStream, the consumer of the stream will call AsyncWait or Available / Read / ReadSegments before the background thread has called InitFromData. In the AsyncWait case this didn't work because for some reason the callback isn't called in this case when encoding is finished. (Maybe nsPNGEncoder doesn't store the callback when it hasn't seen any data yet, I don't remember.) In the sync case, this doesn't work because none of the sync consumers (toDataURL and mozGetAsFile) can handle NS_BASE_STREAM_WOULD_BLOCK.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 28 • 11 years ago

Comment on attachment 714399 [details] [diff] [review]
v1.1

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

Actually, can you split this into an imglib patch and a canvas patch? I can review the canvas patch, but someone else should review the imglib patch (say Joe Drew).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 33 • 11 years ago

Comment on attachment 715624 [details] [diff] [review]
Part 2: canvas patch

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

::: content/html/content/src/HTMLCanvasElement.cpp
@@ +64,5 @@
>    }
>  
> +  virtual ~ToBlobCallback() {}
> +
> +  NS_IMETHOD OnInputStreamReady(nsIAsyncInputStream* aStream)

You seem to be assuming that this gets called once when all data is available. Why is that a safe assumption? Couldn't the encoder make multiple calls to OnInputStreamReady to deliver the data in chunks?

	Joe Drew (not getting mail)
	Comment 34 • 11 years ago

Comment on attachment 715621 [details] [diff] [review]
Part 1: imglib patch

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

> Is it allowed to create an imgIEncoder on the main thread and call its InitWithData on a different thread? Does the interface make such a guarantee anywhere?

It would be much better if the imgIEncoder is interacted with on a single thread only. Setting pointers to a source and destination surface on an event object, as the high quality image scaler does, should be the extent of the interaction between threads. imgIEncoder has threadsafe refcounting, but absolutely nothing else is threadsafe.

What I'm trying to avoid here is having pointers to the imgIEncoder sitting around on the main thread, simply waiting for someone who doesn't know they're actively being used on a different thread and calling methods on the objects. (Pardon me if this is already the case and I simply missed it.)

(It's actually impossible to preallocate the memory for the destination on the main thread, but as long as ownership is clear, passing from one thread to another (and the old thread having no more access to the object), that's OK since jemalloc is threadsafe.)

For your other two questions: I don't know about the toBlob API, so I can't answer; the second question Nick Nethercote can probably answer.

::: image/src/imgTools.cpp
@@ +123,5 @@
> +
> +  NS_IMETHOD Run();
> +
> +private:
> +  nsRefPtr<gfxImageSurface> mImage;

This is going to be an issue. gfx*Surface is not threadsafe in any way; in particular, their refcounting isn't threadsafe. If you want to do this, you need to absolutely guarantee that all gfxImageSurface::AddRef and Release happens on the main thread.

My suggestion is to pass raw uint8_t* s, gotten on the main thread, to the worker.

@@ +400,5 @@
> + , mEncodingFinished(false)
> +{
> +  nsCOMPtr<nsIRunnable> event =
> +    new ImageEncodingEvent(aImage, mEncoder, aEncoderOptions, aFormat, this);
> +  NS_NewThread(getter_AddRefs(mWorkerThread), nullptr);

Is there any reason why you need worker threads for every encoder, instead of a singleton worker thread for all encoders (or a thread pool)?

	Markus Stange [:mstange] Reporter
	Comment 35 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> Comment on attachment 715624 [details] [diff] [review]
> Part 2: canvas patch
> 
> Review of attachment 715624 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/HTMLCanvasElement.cpp
> @@ +64,5 @@
> >    }
> >  
> > +  virtual ~ToBlobCallback() {}
> > +
> > +  NS_IMETHOD OnInputStreamReady(nsIAsyncInputStream* aStream)
> 
> You seem to be assuming that this gets called once when all data is
> available. Why is that a safe assumption? Couldn't the encoder make multiple
> calls to OnInputStreamReady to deliver the data in chunks?

The docs for asyncWait in nsIAsyncInputStream.idl say:

> The
> notification is one-shot, meaning that each asyncWait call will result
> in exactly one notification callback.

But you're right, it could be that encoding hasn't finished when we're notified. I think setting aRequestedCount to UINT32_MAX when calling AsyncWait should fix that - all the encoders notify when finished, regardless of aRequestedCount.

	Markus Stange [:mstange] Reporter
	Comment 36 • 11 years ago

(In reply to Joe Drew (:JOEDREW! \o/) from comment #34)
> Comment on attachment 715621 [details] [diff] [review]
> Part 1: imglib patch
> 
> Review of attachment 715621 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > Is it allowed to create an imgIEncoder on the main thread and call its InitWithData on a different thread? Does the interface make such a guarantee anywhere?
> 
> It would be much better if the imgIEncoder is interacted with on a single
> thread only. Setting pointers to a source and destination surface on an
> event object, as the high quality image scaler does, should be the extent of
> the interaction between threads. imgIEncoder has threadsafe refcounting, but
> absolutely nothing else is threadsafe.

Thanks, this is very helpful.

> What I'm trying to avoid here is having pointers to the imgIEncoder sitting
> around on the main thread, simply waiting for someone who doesn't know
> they're actively being used on a different thread and calling methods on the
> objects. (Pardon me if this is already the case and I simply missed it.)

Well, pointers to the encoder are stored both in the AsyncImageEncoder object and in the ImageEncodingEvent object (called mEncoder in both places) - AsyncImageEncoder is mostly used on the main thread and ImageEncodingEvent on the worker thread. But the encoder can't escape from the AsyncImageEncoder to any consumer, say to the caller of imgTools::EncodeImageData.

> (It's actually impossible to preallocate the memory for the destination on
> the main thread, but as long as ownership is clear, passing from one thread
> to another (and the old thread having no more access to the object), that's
> OK since jemalloc is threadsafe.)

OK.

> ::: image/src/imgTools.cpp
> @@ +123,5 @@
> > +
> > +  NS_IMETHOD Run();
> > +
> > +private:
> > +  nsRefPtr<gfxImageSurface> mImage;
> 
> This is going to be an issue. gfx*Surface is not threadsafe in any way; in
> particular, their refcounting isn't threadsafe. If you want to do this, you
> need to absolutely guarantee that all gfxImageSurface::AddRef and Release
> happens on the main thread.
> 
> My suggestion is to pass raw uint8_t* s, gotten on the main thread, to the
> worker.

Okay. Then we'll need to store the image surface in a RefPtr on the AsyncImageEncoder object; otherwise it will die before encoding starts.

> @@ +400,5 @@
> > + , mEncodingFinished(false)
> > +{
> > +  nsCOMPtr<nsIRunnable> event =
> > +    new ImageEncodingEvent(aImage, mEncoder, aEncoderOptions, aFormat, this);
> > +  NS_NewThread(getter_AddRefs(mWorkerThread), nullptr);
> 
> Is there any reason why you need worker threads for every encoder, instead
> of a singleton worker thread for all encoders (or a thread pool)?

Lifetime management simplicity. But according to comment 19, this will have to change anyway.

If we use a thread pool, we need to be notified on shutdown so that we can destroy the pool, right? What kind of notification should be used here?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 37 • 11 years ago

(In reply to Markus Stange from comment #36)
> If we use a thread pool, we need to be notified on shutdown so that we can
> destroy the pool, right? What kind of notification should be used here?

xpcom-shutdown-threads.

I think you should just use LazyIdleThread and not worry about a thread pool here for now.

	Stephen A Pohl [:spohl] Assignee
	Comment 38 • 11 years ago

This is an attempt at addressing the review feedback. It would be great to know if this is heading in the right direction.

1. We still store a pointer to the imgIEncoder on the main thread, but only interact with it on the worker thread. This is achieved by dispatching events to the worker thread. The ImageEncodingEvent remains an async event. All new events that replace the interaction with mEncoder on the main thread are synchronous events to maintain the behavior of the previous patch.
2. We now use raw gfxImageSurface* pointers on the worker thread. An nsRefPtr to the gfxImageSurface was added to the AsyncImageEncoder class to avoid deletion before the encoding could complete (as suggested by Markus).
3. I was unable to use LazyIdleThread instead of NS_Thread (requested in comment 37). According to comments in LazyIdleThread.cpp: "LazyIdleThread can't always support synchronous dispatch currently." A singleton thread doesn't seem like the right solution from a performance standpoint either, if we keep the synchronous event dispatching described in 1. above. Should I go with a thread pool after all?

	Stephen A Pohl [:spohl] Assignee
	Comment 39 • 11 years ago

This patch sets aRequestedCount to UINT32_MAX when calling AsyncWait, as discussed in comment 35.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 40 • 11 years ago

Comment on attachment 720970 [details] [diff] [review]
Part 2: canvas patch

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

The docs in nsIAsyncInputStream say:
     * @param aRequestedCount
     *        Wait until at least this many bytes can be read.  This is only
     *        a suggestion to the underlying stream; it may be ignored.
So the implementation is allowed to always return, say, one byte. So you need to add code to keep calling asyncWait and concatenating the results until the stream has finished.

	Stephen A Pohl [:spohl] Assignee
	Comment 41 • 11 years ago

This adds nsIAsyncInputStream::WAIT_CLOSURE_ONLY as a flag to the AsyncWait call.

	Stephen A Pohl [:spohl] Assignee
	Comment 42 • 11 years ago

Comment on attachment 721419 [details] [diff] [review]
Part 2: canvas patch

There are failures on try that I'm looking into (https://tbpl.mozilla.org/?tree=Try&rev=d4a623b64b57).
Setting this to feedback? instead of review? while these failures aren't fixed. It would be great to know if the additional flag is acceptable/correct here.

	Stephen A Pohl [:spohl] Assignee
	Comment 43 • 11 years ago

Comment on attachment 720969 [details] [diff] [review]
Part 1: imglib patch

There are failures on try that I'm looking into (https://tbpl.mozilla.org/?tree=Try&rev=d4a623b64b57).
Setting this to feedback? instead of review? while these failures aren't fixed. It would be good to know if this is heading in the right direction though. Thanks!

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 44 • 11 years ago

Comment on attachment 721419 [details] [diff] [review]
Part 2: canvas patch

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

::: content/html/content/src/HTMLCanvasElement.cpp
@@ +82,5 @@
> +    nsRefPtr<nsDOMMemoryFile> blob =
> +      new nsDOMMemoryFile(imgData, imgSize, mType);
> +
> +    // XXX I'd like to call JS_updateMallocCounter here, but I don't know how
> +    // to get the JSContext that ToBlob was called on from here.

I'm pretty sure you can just store the JSContext* in this object. Just make sure that this code doesn't run after the element is destroyed and the callback has been revoked.

@@ +442,5 @@
>  }
>  
> +nsresult
> +HTMLCanvasElement::StartAsyncEncode(nsIAsyncInputStream* aStream,
> +                                      nsIInputStreamCallback* aCallback)

Fix indent

	Josiah Bruner [:jsbruner]
	Comment 45 • 11 years ago

At least patch 1 is broken on the current trunk.

	Stephen A Pohl [:spohl] Assignee
	Comment 46 • 11 years ago

Updated for current trunk.

	Stephen A Pohl [:spohl] Assignee
	Comment 47 • 11 years ago

Addressed feedback from comment 44. Still looking into try failures.

	Joe Drew (not getting mail)
	Comment 48 • 11 years ago

Comment on attachment 721767 [details] [diff] [review]
Part 1: imglib patch

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

In general, this looks like a good approach. There are some specific things I've highlighted below, but I don't see any insurmountable problems.

::: image/src/imgTools.cpp
@@ +68,5 @@
> +  nsCOMPtr<nsIInputStreamCallback> mAsyncWaitCallback;
> +  uint32_t mAsyncWaitFlags;
> +  uint32_t mAsyncWaitRequestedCount;
> +  nsCOMPtr<nsIEventTarget> mAsyncWaitTarget;
> +  ReentrantMonitor mMonitor;

Does this *really* need to be reentrant? It's almost always a sign of code that's going to be incredibly difficult to debug :(

@@ +557,5 @@
> +  nsCOMPtr<nsIRunnable> event = new ImageEncodingEvent(aImage, mEncoder,
> +                                                       aEncoderOptions,
> +                                                       aFormat, this);
> +  NS_NewThread(getter_AddRefs(mWorkerThread), nullptr);
> +  mWorkerThread->Dispatch(event, NS_DISPATCH_NORMAL);

I'm a little worried about creating a thread for every encode (rather than using a thread pool or singleton worker thread), but I don't think it's a dealbreaker.

@@ +653,5 @@
> +}
> +
> +void
> +AsyncImageEncoder::EncodingFinished()
> +{  

extra whitespace

@@ +693,5 @@
> +    nsCOMPtr<nsIRunnable> event = new ImageEncodingAsyncWaitEvent(mEncoder,
> +                                                      callback,
> +                                                      mAsyncWaitFlags,
> +                                                      mAsyncWaitRequestedCount,
> +                                                      mAsyncWaitTarget);

line this up vertically (everywhere)

@@ +728,5 @@
> +ImageEncodingEvent::Run()
> +{
> +  // Do the actual work.
> +  mEncoder->InitFromData(mImage->Data(), mImage->GetDataSize(),
> +                         mImage->Width(), mImage->Height(), mImage->Stride(),

Calling these methods on a different thread is a no-no; better to have raw pointers and values extracted by AsyncImageEncoder and passed to the event.

@@ +759,5 @@
> +                                        uint32_t* _retval)
> +: mEncoder(aEncoder)
> +, mBuf(aBuf)
> +, mCount(aCount)
> +, mRetVal(_retval)

Add an extra space before : and , here

@@ +815,5 @@
> +: mEncoder(aEncoder)
> +, mStatus(aStatus)
> +, mResult(NS_ERROR_NULL_POINTER)
> +{
> +}

A matter of style, but I like putting braces for no-body constructors on the same line
{}

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 49 • 11 years ago

Comment on attachment 721767 [details] [diff] [review]
Part 1: imglib patch

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

::: image/src/imgTools.cpp
@@ +563,5 @@
> +
> +AsyncImageEncoder::~AsyncImageEncoder()
> +{
> +  Close();
> +}

Since Close will spin the event loop I would strongly prefer that you explicitly close the stream rather than rely on the dtor to do so.

We also need to make sure that its safe to spin the event loop everywhere we call Close on this stream.

	Stephen A Pohl [:spohl] Assignee
	Comment 50 • 11 years ago

There are still a number of failures on try. I'm going to post three patches that are meant to ultimately be folded into part 1 and 2. They should be helpful if somebody else wants to take a stab at this.

The results for the latest try run will appear here:
https://tbpl.mozilla.org/?tree=Try&rev=762bed10ebb2

	Stephen A Pohl [:spohl] Assignee
	Comment 54 • 11 years ago

Updated for current trunk.

	Stephen A Pohl [:spohl] Assignee
	Comment 55 • 11 years ago

Removed nsIAsyncInputStream::WAIT_CLOSURE_ONLY from call to AsyncWait. This flag wasn't right, since we wanted to get notified when encoding was finished and we could start reading from the stream. Since we would never get notified about it, we would simply wait indefinitely (since we also wouldn't close the stream, since we hadn't read from it yet). This brings back the problem that the implementation should keep calling AsyncWait when reading from the stream doesn't return all the data (when encoding hasn't finished yet for example). My observations have been that the clients of AsyncImageEncoder do this correctly, or at least our tests don't run into a scenario where this is a problem. I would like to confirm this.

	Stephen A Pohl [:spohl] Assignee
	Comment 56 • 11 years ago

Removes ReentrantMonitor.

	Stephen A Pohl [:spohl] Assignee
	Comment 57 • 11 years ago

This fixes the mozGetAsFile test for our asynchronous implementation. There used to be a race condition between the two tests when reading/writing to the global gCompares variable. Sometimes, the test run would hang because gCompares wouldn't reach 0. Other times, the test would call SimpleTest.finish() more than once.

	Stephen A Pohl [:spohl] Assignee
	Comment 58 • 11 years ago

This fixes the toBlob test for our asynchronous implementation. The same race condition as in the mozGetAsFile test existed here.

	Stephen A Pohl [:spohl] Assignee
	Comment 59 • 11 years ago

This is an attempt at fixing the animSVGImage test. The test used to use an ImageDecoderObserverStub object to periodically check whether the animation had completed or not. With our current asynchronous encoding implementation, this would result in an infinite loop (myOnStopFrame would be called repeatedly), the animation would never complete (the square remained red) and the test would eventually time out and cause a failure. The current fix (which works) is to use setTimeout instead of the observer to check whether the animation has completed. However, I'm concerned that the previous test doesn't work anymore and that I might be missing an issue in our implementation.

	Stephen A Pohl [:spohl] Assignee
	Comment 60 • 11 years ago

This is an attempt at fixing the XHR test. The two calls to SpecialPowers.gc() could result in hangs in the test. I'm concerned that forcing the GC can cause this. When the hang occurs, we seem to never be calling continueTest, which is the FileReader object's onload function. This causes the test to hang and eventually time out.

	Stephen A Pohl [:spohl] Assignee
	Comment 61 • 11 years ago

Although I may have to iterate on some of the proposed fixes to our tests above, we're now down to one (reliable) failure on try, causing a total of about 9 tests to fail:
https://tbpl.mozilla.org/?tree=Try&rev=afbb2c7be94c

This crash occurs in ReleaseSliceNow in XPCJSRuntime.cpp. Based on the latest try run and some additional print statements, a call to AsyncImageEncoder::ShutdownWorkerThread seems to be interrupting a previous call to ReleaseSliceNow, which is still in its for loop. Once the call to ReleaseSliceNow that was triggered by AsyncImageEncoder::ShutdownWorkerThread completes, the length of the array that was processed in the previous ReleaseSliceNow call is out of whack. Here is a short excerpt from the log:

[...]
17:20:05     INFO -  +++++++++++++++++++++++++++++++ itemsLength: 44, lastItemIdx: 43, i: 8327, slice: 8371
17:20:05     INFO -  +++++++++++++++++++++++++++++++ called ElementAt(lastItemIdx)
17:20:05     INFO -  +++++++++++++++++++++++++++++++ called RemoveElementAt(lastItemIdx)
17:20:05     INFO -  +++++++++++++++++++++++++++++++ itemsLength: 43, lastItemIdx: 42, i: 8328, slice: 8371
17:20:05     INFO -  +++++++++++++++++++++++++++++++ called ElementAt(lastItemIdx)
17:20:05     INFO -  +++++++++++++++++++++++++++++++ called RemoveElementAt(lastItemIdx)
17:20:05     INFO -  ********** 0x100522b00 - 0x108307340: AsyncImageEncoder destructor called.
17:20:05     INFO -  ********** Current: 0x100522b00 - isMain: 1: AsyncImageEncoder::ShutdownWorkerThread called.

>>>>>> Next is a call to ReleaseSliceNow that seems to interrupt an already running call

17:20:05     INFO -  +++++++++++++++++++++++++++++++ START with 100
17:20:05     INFO -  +++++++++++++++++++++++++++++++ itemsLength: 42, lastItemIdx: 41, i: 0, slice: 42
17:20:05     INFO -  +++++++++++++++++++++++++++++++ called ElementAt(lastItemIdx)
17:20:05     INFO -  +++++++++++++++++++++++++++++++ called RemoveElementAt(lastItemIdx)
17:20:05     INFO -  +++++++++++++++++++++++++++++++ itemsLength: 41, lastItemIdx: 40, i: 1, slice: 42
17:20:05     INFO -  +++++++++++++++++++++++++++++++ called ElementAt(lastItemIdx)
17:20:05     INFO -  +++++++++++++++++++++++++++++++ called RemoveElementAt(lastItemIdx)

[...]

17:20:05     INFO -  +++++++++++++++++++++++++++++++ itemsLength: 2, lastItemIdx: 1, i: 40, slice: 42
17:20:05     INFO -  +++++++++++++++++++++++++++++++ called ElementAt(lastItemIdx)
17:20:05     INFO -  +++++++++++++++++++++++++++++++ called RemoveElementAt(lastItemIdx)
17:20:05     INFO -  +++++++++++++++++++++++++++++++ itemsLength: 1, lastItemIdx: 0, i: 41, slice: 42
17:20:05     INFO -  +++++++++++++++++++++++++++++++ called ElementAt(lastItemIdx)
17:20:05     INFO -  +++++++++++++++++++++++++++++++ called RemoveElementAt(lastItemIdx)
17:20:05     INFO -  +++++++++++++++++++++++++++++++ END

>>>>>> Once the interrupting call completes, the array length of the previous call is garbage.

17:20:05     INFO -  +++++++++++++++++++++++++++++++ itemsLength: 1282083424, lastItemIdx: 1282083423, i: 8329, slice: 8371
17:20:05  WARNING -  TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/ajax/mochikit/test_Mochikit.html | Exited with code 1 during test run

Full log is available here:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20997262&tree=Try&full=1#error2

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 62 • 11 years ago

Shutting down a thread will spin the event loop, and you absolutely cannot do that while running the GC.  So if you're shutting down threads from a dtor you're probably doing it wrong.

	Stephen A Pohl [:spohl] Assignee
	Comment 63 • 11 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #62)
> Shutting down a thread will spin the event loop, and you absolutely cannot
> do that while running the GC.  So if you're shutting down threads from a
> dtor you're probably doing it wrong.

Thanks! I removed the shutdown of the worker thread in the dtor and added it to Close() and CloseWithStatus(). Unfortunately, I had to realize that we don't guarantee or enforce that Close or CloseWithStatus is called before destroying the object. This is a problem, since the object can basically be destroyed at any time, but we should have shut down the worker thread before the object is being destroyed. I'm not quite sure how to solve this chicken or the egg problem and would appreciate any pointers...

The consequence is that we have threads sitting around that never get closed. In some cases this can lead to out-of-memory conditions like here:
https://tbpl.mozilla.org/php/getParsedLog.php?id=21172731&tree=Try&full=1#error0

The good news is that moving the shutdown of the worker thread out of the dtor fixes the XHR test that patch "Part 9" was trying to fix as well as the issue described in comment 61.

	Stephen A Pohl [:spohl] Assignee
	Comment 64 • 11 years ago

(In reply to Stephen Pohl [:spohl] from comment #63)
> This is a problem, since the object can basically be
> destroyed at any time, but we should have shut down the worker thread before
> the object is being destroyed. I'm not quite sure how to solve this chicken
> or the egg problem and would appreciate any pointers...

I guess if I enforce a Close() on the stream before destroying it, that would do the trick :p. Off to another try run...

	Stephen A Pohl [:spohl] Assignee
	Comment 65 • 11 years ago

Enforcing that a stream is closed before being destroyed does not seem feasible. Client-side JS code may keep a stream around and not necessarily close it, expecting it to destroy itself properly when going out of scope.

I'm starting to think that a thread pool will be necessary to solve this problem. With a thread pool, the AsyncImageEncoder dtor will no longer have to worry about the worker thread and performance should improve as well. Looking into ways to implement a thread pool for our use case now...

	Markus Stange [:mstange] Reporter
	Comment 66 • 11 years ago

Can you initiate thread shutdown from the destructor asynchronously, by posting an nsRunnable? like http://mxr.mozilla.org/mozilla-central/ident?i=ShutdownThreadEvent

	Stephen A Pohl [:spohl] Assignee
	Comment 67 • 11 years ago

(In reply to Markus Stange from comment #66)
> Can you initiate thread shutdown from the destructor asynchronously, by
> posting an nsRunnable? like
> http://mxr.mozilla.org/mozilla-central/ident?i=ShutdownThreadEvent

Markus, I was going to say that I tried that unsuccessfully, only to realize that what I did was:
NS_DispatchToMainThread(NS_NewRunnableMethod(this, &AsyncImageEncoder::ShutdownWorkerThread));

Clearly, that's not a great thing to do in the dtor of AsyncImageEncoder.

By posting an nsRunnable that's unrelated to AsyncImageEncoder in the dtor, we can be sure that worker threads are shut down correctly and a try run with this change looks all green:
https://tbpl.mozilla.org/?tree=Try&rev=4c46c100e00e

Will post updated patches shortly. Thanks Markus!

	Stephen A Pohl [:spohl] Assignee
	Comment 68 • 11 years ago

Robert, could you recommend someone for a review of this patch while Joe is out on vacation? Thanks!

	Stephen A Pohl [:spohl] Assignee
	Comment 69 • 11 years ago

Comment on attachment 729209 [details] [diff] [review]
Part 3: Fix mozGetAsFile test

Please let me know if I should ask someone else for a review of this test.

	Stephen A Pohl [:spohl] Assignee
	Comment 70 • 11 years ago

Comment on attachment 729219 [details] [diff] [review]
Part 4: Fix toBlob test

Please let me know if I should ask someone else for a review of this test.

	Stephen A Pohl [:spohl] Assignee
	Comment 71 • 11 years ago

Comment on attachment 729225 [details] [diff] [review]
Part 5: Fix animSVGImage test

Please let me know if I should ask someone else for a review of this test.

	Stephen A Pohl [:spohl] Assignee
	Comment 72 • 11 years ago

Fixed trailing white space.

Robert, could you recommend someone for a review of this patch while Joe is out on vacation? Thanks!

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 73 • 11 years ago

Comment on attachment 731217 [details] [diff] [review]
Part 1: imglib patch

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

Don't forget to remove


Pending questions:
- Reporting of late errors: Before this patch, errors encountered during encoding, or in the case of too much data after encoding, were propagated to the caller of toBlob. Now they just fall on the ground and the callback is never called. What should be done about this, if anything?
- JS_updateMallocCounter: When OnInputStreamReady fires, we no longer know the JSContext that toBlob was called on. How can we assign the blob's memory correctly?
- Is it allowed to create an imgIEncoder on the main thread and call its InitWithData on a different thread? Does the interface make such a guarantee anywhere?
* * *
Bug 817700: Logging stuff. Don't check inhg diffhg diffhg diff
* * *
Remove ReentrantMonitor

from the commit message.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 74 • 11 years ago

Comment on attachment 729191 [details] [diff] [review]
Part 2: canvas patch

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

I don't see WAIT_CLOSURE_ONLY here

::: content/html/content/src/HTMLCanvasElement.cpp
@@ +122,5 @@
> +  }
> +
> +  NS_IMETHOD OnInputStreamReady(nsIAsyncInputStream* aStream)
> +  {
> +    nsresult rv = NS_OK;

Add an assertion that this is on the main thread.

	Stephen A Pohl [:spohl] Assignee
	Comment 75 • 11 years ago

Updated commit message.

	Stephen A Pohl [:spohl] Assignee
	Comment 76 • 11 years ago

Added assert. WAIT_CLOSURE_ONLY does not appear to be the correct flag for what we want to do. Please see comment 55 for more details.

This is my understanding of the problem we're trying to solve with regards to OnInputStreamReady:
1. The current patch makes the assumption that when OnInputStreamReady is called, all data is available and can be read from the stream.
2. The encoder may however deliver data in chunks and call OnInputStreamReady multiple times. In this scenario, we should keep calling AsyncWait on the encoder until all data was made available by the encoder.

Our tests don't seem to exhibit scenario 2. If this is indeed a valid scenario, how should I determine when all data was made available by the encoder? Should I add up the number of bytes read from the stream until it matches what was returned by Available()?

	Seth Fowler [:seth] [:s2h]
	Comment 77 • 11 years ago

Comment on attachment 731963 [details] [diff] [review]
Part 1: imglib patch

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

::: image/public/imgITools.idl
@@ +171,5 @@
> +     */
> +    [noscript] nsIInputStream encodeImageData(in gfxImageSurface aSurface,
> +                                              in ACString aMimeType,
> +                                              in AString aOutputOptions,
> +                                              in uint32_t aFormat);

My advice is not to place this in imgITools at all, since it's not intended to be accessible from JavaScript. (Note that this would be the first noscript function in imgITools.) Just add a new set of files (image/src/ImageEncoder.{h|cpp}, maybe?) and add them to the list of the module's exports. You'll then be able to present a cleaner API to C++ callers, avoid adding more XPCOM cruft to the codebase unnecessarily, and place the implementation in its own file.

::: image/src/imgTools.cpp
@@ +83,5 @@
> +  virtual ~StreamCallbackWrapper() {}
> +
> +  NS_IMETHOD OnInputStreamReady(nsIAsyncInputStream* aStream) {
> +    // Ignore aStream, use mWrapperStream instead.
> +    // That's the only purpose of this class.

You should comment out the aStream identifier: "NS_IMETHOD OnInputStreamReady(nsIAsyncInputStream* /* aStream */)". If you don't, this will cause unused variable compiler warnings.

@@ +95,5 @@
> +
> +/**
> + * A runnable that will be run on the encoder thread.
> + * It takes owning references to the image surface, the encoder, and the
> + * AsyncImageEncoder stream.

Doesn't it actually take _weak_ references? Somebody else has to keep this stuff alive, right? It'd be good to state that explicitly anywhere it's happening.

@@ +276,5 @@
> +class ShutdownThreadEvent : public nsRunnable
> +{
> +public:
> +  ShutdownThreadEvent(nsIThread* aThread) : mThread(aThread) {}
> +  ~ShutdownThreadEvent() {}

Don't bother defining an empty destructor.

@@ +347,5 @@
>    NS_ENSURE_TRUE(length <= UINT32_MAX, NS_ERROR_FILE_TOO_BIG);
>  
>    // Send the source data to the Image.
> +  rv = image->OnImageDataAvailable(nullptr, nullptr, inStream, 0,
> +                                   uint32_t(length));

Looks like nothing has changed here but formatting. I'd probably just restore the original version so as not to pollute hg blame.

@@ +386,5 @@
>    NS_ENSURE_ARG(aScaledWidth >= 0 && aScaledHeight >= 0);
>  
>    // If no scaled size is specified, we'll just encode the image at its
>    // original size (no scaling).
> +  if (aScaledWidth == 0 && aScaledHeight == 0)

Also wouldn't pollute hg blame here just to remove these braces, which are imagelib style anyway. There are other cases of blame pollution in this patch which I'd also remove.

@@ +495,5 @@
>    nsCOMPtr<imgIEncoder> encoder = do_CreateInstance(encoderCID.get());
>    if (!encoder)
>      return NS_IMAGELIB_ERROR_NO_ENCODER;
>  
> +  nsCOMPtr<AsyncImageEncoder> asyncEncoderStream =

This should be an nsRefPtr.

@@ +581,5 @@
> +}
> +
> +AsyncImageEncoder::~AsyncImageEncoder()
> +{
> +  nsCOMPtr<nsIRunnable> event = new ShutdownThreadEvent(mWorkerThread);

Doesn't AsyncImageEncoder::mImage need to stay alive until the worker thread is _actually_ shut down? It seems to me like you should pass it to ShutdownThreadEvent so it can keep it alive until that point.

@@ +595,5 @@
> +  mAsyncWaitCallback = nullptr;
> +  mAsyncWaitTarget = nullptr;
> +  nsRefPtr<ImageEncodingCloseEvent> event =
> +    new ImageEncodingCloseEvent(mEncoder);
> +  mWorkerThread->Dispatch(event, NS_DISPATCH_SYNC);

So this is super bad. This will spin the event loop waiting for the event to run on the other thread, which causes all sorts of problems. Unfortunately, this is happening throughout this class.

You essentially have two options here. You can use SyncRunnable, which will let you retain this synchronous API but will block the main thread until the event runs.  Obviously that's going to limit your potential for concurrency a lot, and the snappy folks will (I am told) send vampires after you, so I'd consider this an undesirable approach. If you have requirements that force your hand here, make sure you can defend the decision.

The second option is much better, and unless you absolutely cannot you should take it. Rethink this patch so you don't have to conform to a synchronous API. You could make this behave as a proper non-blocking nsIAsyncInputStream, updating callers as needed so they can handle this. Or you could update the callers to use a different approach that also wouldn't require blocking the main thread.

@@ +691,5 @@
> +                                    mAsyncWaitFlags,
> +                                    mAsyncWaitRequestedCount,
> +                                    mAsyncWaitTarget);
> +  if (mWorkerThread)
> +    mWorkerThread->Dispatch(event, NS_DISPATCH_SYNC);

AsyncWait seems like a particularly bizarre thing to sync dispatch...

@@ +695,5 @@
> +    mWorkerThread->Dispatch(event, NS_DISPATCH_SYNC);
> +  else
> +    event->Run();
> +  mAsyncWaitCallback = nullptr;
> +  mAsyncWaitTarget = nullptr;

I don't understand why mAsyncWaitCallback and mAsyncWaitTarget need to be member variables. (Nor why they're set to null in e.g. the Close* methods.) If you continue to dispatch synchronously, AFAICT we can just store them on the stack. Otherwise you probably need to use a different approach, but this doesn't seem like the right one.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 78 • 11 years ago

(In reply to Stephen Pohl [:spohl] from comment #76)
> Added assert. WAIT_CLOSURE_ONLY does not appear to be the correct flag for
> what we want to do. Please see comment 55 for more details.

Shouldn't the encoder be closing the stream once it's finished writing data?

> This is my understanding of the problem we're trying to solve with regards
> to OnInputStreamReady:
> 1. The current patch makes the assumption that when OnInputStreamReady is
> called, all data is available and can be read from the stream.
> 2. The encoder may however deliver data in chunks and call
> OnInputStreamReady multiple times. In this scenario, we should keep calling
> AsyncWait on the encoder until all data was made available by the encoder.
> 
> Our tests don't seem to exhibit scenario 2.

We need to code to the contract for nsIAsyncInputStream, not whatever our encoders currently happen to do.

> If this is indeed a valid
> scenario, how should I determine when all data was made available by the
> encoder? Should I add up the number of bytes read from the stream until it
> matches what was returned by Available()?

It seems to me the encoder should close the stream when it has finished producing data. As far as I can tell, that's the only reliable signal there is no more data to read.

	Stephen A Pohl [:spohl] Assignee
	Comment 79 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #78)
> (In reply to Stephen Pohl [:spohl] from comment #76)
> > Added assert. WAIT_CLOSURE_ONLY does not appear to be the correct flag for
> > what we want to do. Please see comment 55 for more details.
> 
> Shouldn't the encoder be closing the stream once it's finished writing data?
> 

I should probably have said that the flag is not currently doing what it's supposed to. Just by checking our PNG encoder it appears that we return early with NS_ERROR_NOT_IMPLEMENTED whenever we try to pass a flag to AsyncWait:
http://mxr.mozilla.org/mozilla-central/source/image/encoders/png/nsPNGEncoder.cpp#554

I can either implement this for our encoders, or update the callers so that they will keep calling AsyncWait until all data could be read from the stream. While I'm addressing the feedback in comment 77 I'll look into both options, but please let me know if one or the other is definitely the way to go.

	Stephen A Pohl [:spohl] Assignee
	Comment 80 • 11 years ago

This is a new approach to this bug. I've been inspired by comment 18 and crushed by comment 77, which culminated in this patch. The patch intentionally focuses on <canvas>.toBlob() only since the bug was originally written that way. <canvas>.toBlob() is also what blocks bug 678392 and bug 673875. If this is an acceptable approach I'll gladly file new bugs to address the remaining methods in HTMLCanvasElement.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 81 • 11 years ago

How does this patch address the issue of OnInputStreamReady firing before all the data has been produced?

	Stephen A Pohl [:spohl] Assignee
	Comment 82 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #81)
> How does this patch address the issue of OnInputStreamReady firing before
> all the data has been produced?

It doesn't. I was just trying to fix the bug here, which is to run <canvas>.toBlob on a worker thread. Unless I'm reading this wrong, I believe this will not amplify the existing risk that OnInputStreamReady can be called before data is available. Is this wrong?

I agree though that we should address this problem too and if agreed, I'd open a new bug for that.

	Stephen A Pohl [:spohl] Assignee
	Comment 83 • 11 years ago

(In reply to Stephen Pohl [:spohl] from comment #82)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #81)
> > How does this patch address the issue of OnInputStreamReady firing before
> > all the data has been produced?
> 
> It doesn't. I was just trying to fix the bug here, which is to run
> <canvas>.toBlob on a worker thread. Unless I'm reading this wrong, I believe
> this will not amplify the existing risk that OnInputStreamReady can be
> called before data is available. Is this wrong?
> 
> I agree though that we should address this problem too and if agreed, I'd
> open a new bug for that.

Err... Please ignore. For some reason I thought that the existing toBlob implementation was using AsyncWait/OnInputStreamReady as well. I'll look into it.

	Stephen A Pohl [:spohl] Assignee
	Comment 84 • 11 years ago

This patch maintains the existing toBlob behavior but moves it to a worker thread (without the use of AsyncWait/OnInputStreamReady).

The problem with AsyncWait/OnInputStreamReady is not straightforward to solve for various reasons:
1. If we use an OnInputStreamReady callback to detect when the write phase is complete (like earlier patches did), then these callbacks could (theoretically) be called multiple times when more data became available.
2. The WAIT_CLOSURE_ONLY flag was not a solution even after making encoders aware of this flag, because if we closed the stream after the write phase to trigger the OnInputStreamReady callback, the stream would now be closed and we could no longer read from it (see behavior of Available, Read and ReadSegments when called on closed streams).

FYI: I also tried to move the call to ExtractData to the worker thread. Unfortunately, this is not (easily) possible because ExtractData could indirectly call mozilla::image::DiscardTracker::Initialize, which is only permitted to be called on the main thread:
https://tbpl.mozilla.org/?tree=Try&rev=cc5d65efde9a

A try run with the patch here applied is all green:
https://tbpl.mozilla.org/?tree=Try&rev=abc2ae7c7a3f

	Stephen A Pohl [:spohl] Assignee
	Comment 85 • 11 years ago

Removed trailing white space.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 86 • 11 years ago

Comment on attachment 735983 [details] [diff] [review]
Part 1: Patch

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

::: content/html/content/public/HTMLCanvasElement.h
@@ +177,5 @@
>    nsRefPtr<HTMLCanvasElement> mOriginalCanvas;
>    nsCOMPtr<nsIPrintCallback> mPrintCallback;
>    nsCOMPtr<nsICanvasRenderingContextInternal> mCurrentContext;
>    nsCOMPtr<HTMLCanvasPrintState> mPrintState;
> +  nsCOMPtr<nsIThread> mWorkerThread;

Should be called mEncoderThread I think

::: content/html/content/src/HTMLCanvasElement.cpp
@@ +699,5 @@
> +  nsRefPtr<ToBlobRunnable> event = new ToBlobRunnable(aCallback,
> +                                                      stream,
> +                                                      type,
> +                                                      context);
> +  return mWorkerThread->Dispatch(event, NS_DISPATCH_NORMAL);

I'm confused ...how do you know the encoder has finished producing all the data by the time this event runs?

	Stephen A Pohl [:spohl] Assignee
	Comment 87 • 11 years ago

Hi Robert, this patch demonstrates the new approach. Rather than making the interaction with the encoder async, we move the synchronous encoding behavior to a worker thread from within toBlob.

I realize that there are a number of threading issues with this approach. For example, we're interacting with an HTMLCanvasElement from multiple threads. Try runs seem to confirm that there are numerous issues. Before I'm trying to fix all the problems, it'd be great to know if this is even worth it. The most recent try run is available here:
https://tbpl.mozilla.org/?tree=Try&rev=8c5f560abd44

If the consensus is that this new approach is not acceptable, at least I know that I can focus all my attention on getting the previous patches (comment 75 and comment 76) up and going.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 88 • 11 years ago

Can't you do the ExtractData on the main thread, and do the call to NS_ReadInputStreamToBuffer in the runnable? That should work. The problem all along has been that you don't read until you reach the end of the stream, and NS_ReadInputStreamToBuffer takes care of that.

	Stephen A Pohl [:spohl] Assignee
	Comment 89 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #88)
> Can't you do the ExtractData on the main thread, and do the call to
> NS_ReadInputStreamToBuffer in the runnable? That should work. The problem
> all along has been that you don't read until you reach the end of the
> stream, and NS_ReadInputStreamToBuffer takes care of that.

What you describe is exactly what my previous patch in comment 85 was doing. Based on a few tests that I did I thought that this approach wouldn't give us the performance gains that we were hoping for, since this approach only moved the read phase to the worker, leaving the (what I thought to be) expensive write phase on the main thread. To get a clearer picture, I submitted the different patches to try this morning. These were the four submits:

1. https://hg.mozilla.org/try/rev/348db5f05505: Enabled bug 678392, leaving <canvas>.toBlob in its original state (no patches from bug 817700 applied.
2. https://hg.mozilla.org/try/rev/32e5cb842a08: Same as 1 above plus patch from comment 85 applied. This only moves the read phase (NS_ReadInputStreamToBuffer) to the worker.
3. https://hg.mozilla.org/try/rev/ef450d7b513a: Same as 1 above plus patch from comment 87 applied. This moves both the write (ExtractData) and read phase (NS_ReadInputStreamToBuffer) to the worker.
4. https://hg.mozilla.org/try/rev/846308ed553f: Same as 1 above plus the patches from comment 75 and comment 76 applied. This is the previous approach using AsyncImageEncoder.

The tbpl link for the last submit (4) is:
https://tbpl.mozilla.org/?tree=Try&rev=846308ed553f
By expanding the submits at the bottom, the three other submits will be revealed right before this one.

If the performance of submit 2 is good enough, I'd go ahead and ask for a review of that patch. I'd appreciate some assistance in making that determination.

If submit 2 isn't good enough, I'd appreciate a recommendation whether I should go with the approach in submit 3 or 4.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 90 • 11 years ago

Attempt #2 didn't work. #3 is giving the best results, but even there, it's a significant regression from mozilla-central.

Can we set up the entire encoding process to run off the main thread? i.e. make a copy of the canvas data, pass it to another thread to complete the encoding, and then pass the results back?

	Stephen A Pohl [:spohl] Assignee
	Comment 91 • 11 years ago

This patch is implementing roc's suggestion from comment 90: We create a copy of the surface on the main thread. This surface is passed to the encoder thread. The encoded result is passed back to the main thread.

I've kicked off a few try builds to compare performance. I'd appreciate some feedback on these numbers since I'm struggling to find a good way to compare the results myself:

1. https://tbpl.mozilla.org/?tree=Try&rev=5b12e5ed96c3: This try run had only this patch (bug 817700) applied. We expect performance to be equal or better than current trunk.

2. https://tbpl.mozilla.org/?tree=Try&rev=4044c87f0773: This try run had only the patches for bug 678392 enabled. We expect performance to be worse than current trunk.

3. https://tbpl.mozilla.org/?tree=Try&rev=80c133bdbe36: This try run had this patch (bug 817700) applied and also enabled the patches for bug 678392. We expect performance to lie somewhere between 1 and 2 above and hopefully be acceptable enough in order to enable history swipe animations (bug 678392) by default on Mac OSX.



I've also kicked off a try build to make sure that all tests pass on all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=4c0310538216

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 92 • 11 years ago

Comment on attachment 743824 [details] [diff] [review]
Part 1: Patch

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

This looks like the right approach, but I think we should split out the encoding code.

How about splitting this up into two patches:
1) a patch to add a new method to imgITools that takes a gfxASurface and a callback object. This method would copy the gfxASurface to an internal buffer, encode the buffer off the main thread, and when that's done, call the callback on the main thread, transferring ownership of the encoded data to the callback. I think this interface should just be in C++ but imgITools.idl is probably still the right place for it, just use a C++ block.
2) a patch to hook up that interface to the canvas.

It's important that we ensure we only make one copy of the canvas's gfxASurface.

	Stephen A Pohl [:spohl] Assignee
	Comment 93 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #92)
> This looks like the right approach, but I think we should split out the
> encoding code.

Sounds good.

> 1) a patch to add a new method to imgITools that takes a gfxASurface and a
> callback object. This method would copy the gfxASurface to an internal
> buffer, encode the buffer off the main thread, and when that's done, call
> the callback on the main thread, transferring ownership of the encoded data
> to the callback. I think this interface should just be in C++ but
> imgITools.idl is probably still the right place for it, just use a C++ block.

At the beginning of comment 77, :seth seemed to say that we should not place this in imgITools.idl but in separate files, since it would be the first [noscript] function in the interface. Would you agree?


> It's important that we ensure we only make one copy of the canvas's
> gfxASurface.

The great thing here is that we always used to make a copy (even before this bug). AFAICT, we don't need more than this one copy.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 94 • 11 years ago

(In reply to Stephen Pohl [:spohl] from comment #93)
> At the beginning of comment 77, :seth seemed to say that we should not place
> this in imgITools.idl but in separate files, since it would be the first
> [noscript] function in the interface. Would you agree?

Sure.

	Stephen A Pohl [:spohl] Assignee
	Comment 95 • 11 years ago

This adds two new files (as suggested in comment 77) with helper functions to encode images synchronously and asynchronously.

	Stephen A Pohl [:spohl] Assignee
	Comment 96 • 11 years ago

Canvas changes

	Stephen A Pohl [:spohl] Assignee
	Comment 97 • 11 years ago

I submitted a few configurations to try again:

1. bee1aa5232f3: Only the patches here applied (bug 817700).
2. 1c48551ef63e: Only history swipe animations enabled (bug 678392).
3. 6c5a5071519f: Both the patches here and swipe animations enabled (bug 817700 + bug 678392).

Again, the hope is that 1. does not have a negative effect on performance, and that 3. is significantly better than 2. Ideally, the performance of 3. is sufficient to turn history swipe animations on by default.

	Seth Fowler [:seth] [:s2h]
	Comment 98 • 11 years ago

Comment on attachment 748988 [details] [diff] [review]
Part 1: imglib patch

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

Wow, total rewrite. Looks a _lot_ better! Mostly I just have nits at this point. =)

One general comment: I don't understand why you have to shut down the encoding thread from the main thread. I may be missing something, but my understanding is that it's sufficient to call nsIThread::Shutdown from a _different_ thread (that is, from a thread other than the one you want to shut down). This suggests to me that it should be OK to call it directly in EncodingCompleteEvent and get rid of ShutdownThreadEvent. I haven't read through all of the comments on this bug, so I apologize if you've explained this elsewhere.

::: image/src/ImageEncoder.cpp
@@ +224,5 @@
> +                                  imgIEncoder* aEncoder,
> +                                  imgIEncoder* aFallbackEncoder)
> +{
> +  nsCOMPtr<nsICanvasRenderingContextInternal> context = aContext;
> +  nsRefPtr<gfxImageSurface> emptyCanvas;

Style nit: you only use emptyCanvas in one |if| branch below. I suggest just declaring it in that branch to keep its scope as narrow as possible.

@@ +293,5 @@
> +  return NS_OK;
> +}
> +
> +/* static */
> +nsresult ImageEncoder::GetImageEncoders(const nsAString& aType,

Style nit: imagelib style is to keep the return type on its own line.

::: image/src/ImageEncoder.h
@@ +39,5 @@
> +                                   nsIFileCallback* aCallback);
> +
> +  // This is called by ExtractData and (indirectly) by ExtractDataAsync above.
> +  // Although permissible, there shouldn't be a need for clients to call this
> +  // directly. Use ExtractData and ExtractDataAsync above instead.

It's a nit, but if you really think nobody should need to call ExtractDataInternal, I'd just mark it private.

@@ +52,5 @@
> +                      imgIEncoder* aEncoder,
> +                      imgIEncoder* aFallbackEncoder);
> +
> +protected:
> +  static nsresult GetImageEncoders(const nsAString& aType,

I'd suggest marking this as private for sure. Generally people don't inherit from static utility classes, so protected doesn't make much sense.

	Stephen A Pohl [:spohl] Assignee
	Comment 99 • 11 years ago

Thank you, Seth, for the fast review! Here is a revised patch. I'm setting it to r? to make sure that it's still deserving of the r+ after my comments below. :-)

(In reply to Seth Fowler [:seth] from comment #98)
> One general comment: I don't understand why you have to shut down the
> encoding thread from the main thread. I may be missing something, but my
> understanding is that it's sufficient to call nsIThread::Shutdown from a
> _different_ thread (that is, from a thread other than the one you want to
> shut down). This suggests to me that it should be OK to call it directly in
> EncodingCompleteEvent and get rid of ShutdownThreadEvent. I haven't read
> through all of the comments on this bug, so I apologize if you've explained
> this elsewhere.

I believe this survived my revisions and isn't needed anymore. This patch implements your suggestion. Thanks for catching!

> ::: image/src/ImageEncoder.cpp
> @@ +224,5 @@
> > +                                  imgIEncoder* aEncoder,
> > +                                  imgIEncoder* aFallbackEncoder)
> > +{
> > +  nsCOMPtr<nsICanvasRenderingContextInternal> context = aContext;
> > +  nsRefPtr<gfxImageSurface> emptyCanvas;
> 
> Style nit: you only use emptyCanvas in one |if| branch below. I suggest just
> declaring it in that branch to keep its scope as narrow as possible.

The |if| branch is inside a while loop (which gets executed at most twice). I was trying to avoid creating a new gfxImageSurface twice if we fall back to the fallback encoder. If it would be preferable to move this declaration inside the while loop and risk allocating a new gfxImageSurface twice, I can certainly do that.
 
> ::: image/src/ImageEncoder.h
> @@ +39,5 @@
> > +                                   nsIFileCallback* aCallback);
> > +
> > +  // This is called by ExtractData and (indirectly) by ExtractDataAsync above.
> > +  // Although permissible, there shouldn't be a need for clients to call this
> > +  // directly. Use ExtractData and ExtractDataAsync above instead.
> 
> It's a nit, but if you really think nobody should need to call
> ExtractDataInternal, I'd just mark it private.

This is actually called from the EncodingRunnable's Run() method in the async encoding scenario. EncodingRunnable wouldn't have access to ExtractDataInternal if it wasn't public. If there is a better way to do this, or if I should change the comment to reflect this, please let me know.

	Seth Fowler [:seth] [:s2h]
	Comment 100 • 11 years ago

(In reply to Stephen Pohl [:spohl] from comment #99)
> The |if| branch is inside a while loop (which gets executed at most twice).
> I was trying to avoid creating a new gfxImageSurface twice if we fall back
> to the fallback encoder. If it would be preferable to move this declaration
> inside the while loop and risk allocating a new gfxImageSurface twice, I can
> certainly do that.

I think you made the right choice here! Apologies for the false alarm; misread the code.

> This is actually called from the EncodingRunnable's Run() method in the
> async encoding scenario. EncodingRunnable wouldn't have access to
> ExtractDataInternal if it wasn't public. If there is a better way to do
> this, or if I should change the comment to reflect this, please let me know.

Ah, if that's the reason you've made it public, then just forward declare EncodingRunnable in ImageEncoder.h and make it a friend of ImageEncoder.

	Seth Fowler [:seth] [:s2h]
	Comment 101 • 11 years ago

Comment on attachment 749039 [details] [diff] [review]
Part 1: imglib patch

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

r+ with the change discussed in the previous comment to make ExtractDataInternal private.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 102 • 11 years ago

Comment on attachment 748989 [details] [diff] [review]
Part 2: canvas patch

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

::: content/canvas/public/nsICanvasRenderingContextInternal.h
@@ +66,5 @@
>                      gfxPattern::GraphicsFilter aFilter,
>                      uint32_t aFlags = RenderFlagPremultAlpha) = 0;
>  
> +  // Creates an image buffer from the thebes surface.
> +  NS_IMETHOD GetImageBuffer(uint8_t** aImageBuffer) = 0;

This can just return uint8_t* directly. No need for the horrid XPCOM pattern.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +959,5 @@
>    return rv;
>  }
>  
> +nsresult
> +CanvasRenderingContext2D::GetSurface(gfxASurface** aSurface)

How is this different from GetThebesSurface? I don't think it is. Maybe you should add the if (!IsTargetValid()) code to GetThebesSurface.

@@ +1016,5 @@
> +{
> +  uint8_t* rawBuffer;
> +  nsresult rv = GetImageBuffer(&rawBuffer);
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_FAILURE;

return rv;

@@ +1024,5 @@
> +  nsCString enccid("@mozilla.org/image/encoder;2?type=");
> +  enccid += aMimeType;
> +  nsCOMPtr<imgIEncoder> encoder = do_CreateInstance(enccid.get(), &rv);
> +  if (NS_FAILED(rv) || !encoder) {
> +    return NS_FAILED(rv) ? rv : NS_ERROR_FAILURE;

I wouldn't bother. I'd just write
  if (!encoder) {
    return NS_ERROR_FAILURE;
  }

@@ +1028,5 @@
> +    return NS_FAILED(rv) ? rv : NS_ERROR_FAILURE;
> +  }
> +
> +  return GetInputStream(
> +    mWidth, mHeight, imageBuffer, encoder, aEncoderOptions, aStream);

Why not inline the other GetInputStream into this method? Having those as separate methods doesn't seem to help much.

	Stephen A Pohl [:spohl] Assignee
	Comment 103 • 11 years ago

Declaring EncodingRunnable class as friend of ImageEncoder and making ExtractDataInternal private. Carrying over r+. Thanks Seth!

	Stephen A Pohl [:spohl] Assignee
	Comment 104 • 11 years ago

Thanks, roc, for the fast review!

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #102)
> Comment on attachment 748989 [details] [diff] [review]
> Part 2: canvas patch
> 
> Review of attachment 748989 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +1028,5 @@
> > +    return NS_FAILED(rv) ? rv : NS_ERROR_FAILURE;
> > +  }
> > +
> > +  return GetInputStream(
> > +    mWidth, mHeight, imageBuffer, encoder, aEncoderOptions, aStream);
> 
> Why not inline the other GetInputStream into this method? Having those as
> separate methods doesn't seem to help much.

The static method exists so that it can be called from EncodingRunnable (on the encoding thread) in ImageEncoder.cpp (see patch part 1).

I kicked off a try run to make sure that everything's still green:
https://tbpl.mozilla.org/?tree=Try&rev=e5f30649345c

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 105 • 11 years ago

Comment on attachment 749519 [details] [diff] [review]
Part 2: canvas patch

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

::: content/canvas/public/nsICanvasRenderingContextInternal.h
@@ +65,5 @@
>    NS_IMETHOD Render(gfxContext *ctx,
>                      gfxPattern::GraphicsFilter aFilter,
>                      uint32_t aFlags = RenderFlagPremultAlpha) = 0;
>  
> +  // Creates an image buffer from the thebes surface.

Add "// Returns null on failure"

	Stephen A Pohl [:spohl] Assignee
	Comment 106 • 11 years ago

Added comment. Carrying over r+.

	Stephen A Pohl [:spohl] Assignee
	Comment 107 • 11 years ago

(In reply to Stephen Pohl [:spohl] from comment #104)
> I kicked off a try run to make sure that everything's still green:
> https://tbpl.mozilla.org/?tree=Try&rev=e5f30649345c

The try run above was lost during this afternoon's tbpl outage. I kicked off a new run and all is green:
https://tbpl.mozilla.org/?tree=Try&rev=a9d615ab345e

	Ryan VanderMeulen [:RyanVM]
	Comment 108 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6bddc947d38
https://hg.mozilla.org/integration/mozilla-inbound/rev/06ae8895852b
https://hg.mozilla.org/integration/mozilla-inbound/rev/44a748256eca
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a3535c6e652
https://hg.mozilla.org/integration/mozilla-inbound/rev/fad852b5819a

	Ryan VanderMeulen [:RyanVM]
	Comment 109 • 11 years ago

Backed out for consistent Ubuntu32 PGO timeouts in premultiplyalpha-test.html of the webgl conformance suite.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd6c6f5e554

https://tbpl.mozilla.org/php/getParsedLog.php?id=22990081&tree=Mozilla-Inbound

	Mike Hommey [:glandium]
	Comment 110 • 11 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #109)
> Backed out for consistent Ubuntu32 PGO timeouts in
> premultiplyalpha-test.html of the webgl conformance suite.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd6c6f5e554
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=22990081&tree=Mozilla-
> Inbound

Note gcc 4.7 builds don't need pgo to consistently trigger webgl conformance oranges. See bug 872960.

	Asa Dotzler [:asa]
	Comment 111 • 11 years ago

Stephen, what's the status here. It looks like this got backed out for a test failure and then nothing.

	Stephen A Pohl [:spohl] Assignee
	Comment 112 • 11 years ago

(In reply to Asa Dotzler [:asa] from comment #111)
> Stephen, what's the status here. It looks like this got backed out for a
> test failure and then nothing.

Two things happened with this bug:
1. It got reprioritized and is now second to the overlay scrollbars (bug 636564). The remaining issues and current status are being tracked here: https://wiki.mozilla.org/Lion_Scrollbars/Triage
2. Although the backout seemingly occurred 'just' because of a test failure it actually revealed a greater underlying issue. The fact that tests started to fail with pgo and/or gcc 4.7 is most likely a sign that our patch has problems that need to be addressed first (note that try isn't building with pgo or gcc 4.7 by default, so this went undetected until we landed the patch). It may be that we have race conditions that our tests aren't usually detecting (unless they're run on a build with pgo or gcc 4.7). This needs some investigation which I'm eager to get to once the fallout for the overlay scrollbars got fixed.

I hope this makes sense.

	Asa Dotzler [:asa]
	Comment 113 • 11 years ago

(In reply to Stephen Pohl [:spohl] from comment #112)
> I hope this makes sense.

Yes. Thank you for the update.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 114 • 11 years ago

Moving this to tracking FF24 so it can get bake time on Aurora and, if successful, be relnoted in FF24 Beta notes (as bug 860493)

	bhavana bajaj [:bajaj]
	Comment 115 • 11 years ago

Based on comment 14, this did not make it into Fx24 for Beta so I am flipping the tracking flag for Fx25 instead and hoping this will be resolved then.

	Stephen A Pohl [:spohl] Assignee
	Comment 116 • 11 years ago

The changes made in this patch aren't particularly imgLib specific, but help in understanding the changes in the canvas patch that I will post shortly, so I'm sending this to :roc. Rob, I'm hoping that bugzilla's patch-diff functionality will be sufficient to review the changes from the previous imgLib patch. If you'd prefer that I attach a separate diff file that highlights the changes, just let me know.

These are the things that were added/changed:
- Updated for current trunk.
- Added fallback to EncodingRunnable when faced with unrecognized custom parse options. This was existing behavior but was missed in earlier patches for this bug.
- Added mFormat property to EncodingRunnable (used by WebGLContext where both imgIEncoder::INPUT_FORMAT_HOSTARGB and imgIEncoder::INPUT_FORMAT_RGBA are possible).
- Added a static ImageEncoder::GetInputStream function that replaces the static CanvasRenderingContext2D::GetInputStream method, since this is used by both CanvasRenderingContext2D and WebGLContext.
- Started to store (in rv) the nsresult of the call to context->GetInputStream in ImageEncoder::ExtractDataInternal. This was partially responsible for the failures that lead to the backout of earlier patches. context->GetInputStream is called in the ToDataURL case, since it isn't asynchronous (yet).

	Stephen A Pohl [:spohl] Assignee
	Comment 117 • 11 years ago

(Setting to feedback? rather than review? for now due to pending decision about the GetImageBuffer method in the WebGLContext case, see bottom of comment)

This patch fixes the failures that lead to the backout of the previous patches. Rob, as mentioned previously, if the bugzilla patch-diff functionality isn't sufficient to review the differences between this and the previous patch, just let me know and I'll upload a separate diff file. Thanks!

These are the things that were added/changed:
- Updated for current trunk.
- Replaced delete with delete[] in CanvasRenderingContext2D::GetImageBuffer.
- Changed prototype of GetImageBuffer to also return the format used (needed when the context is a WebGLContext).
- Removed the static CanvasRenderingContext2D::GetInputStream method and added a static ImageEncoder::GetInputStream function, since this is used by both CanvasRenderingContext2D and WebGLContext.
- Added image/src directory to content/canvas/Makefile.in to avoid relative include paths in cpp files.
- Started passing custom parse options to ImageEncoder::ExtractDataAsync. This is important for JPEG quality parameters for example.
- Implemented WebGLContext::GetImageBuffer (this unimplemented method caused the WebGL conformance test errors and PGO failures that resulted in the backout of the previous patches).

One question remains about WebGLContext::GetImageBuffer: Currently, we do a memcpy at the end of the method to keep nsRefPtr<gfxImageSurface>'s mData around. I've tried using the gfxImageSurface(unsigned char *aData, const gfxIntSize& aSize, long aStride, gfxImageFormat aFormat) constructor as in CanvasRenderingContext2D::GetImageBuffer to create a gfxImageSurface object around an existing/pre-allocated data buffer. However, I found that when the nsRefPtr<gfxImageSurface> and nsRefPtr<gfxContext> go out of scope, the contents of the pre-allocated buffer still change and cause test failures. There are various ways to address this:
1. Figure out why the data buffer changes and fix it if we are positive that it shouldn't change. If it is permissible for the data buffer to change, I wonder if our assumptions in CanvasRenderingContext2D::GetImageBuffer are dangerous and we can't rely on the buffer to remain unchanged.
2. Instead of returning just a uint8_t* buffer, return an already_AddRefed gfxImageSurface object. I believe this should be fine even across multiple threads in the toBlob case.
3. Use PodCopy instead of memcpy. I was told on IRC that this is preferred over memcpy, but wanted to confirm.
4. Keep using memcpy as this patch already does.

	Stephen A Pohl [:spohl] Assignee
	Comment 118 • 11 years ago

Green try run: https://tbpl.mozilla.org/?tree=Try&rev=fb26ba7a6200
Green PGO run: https://tbpl.mozilla.org/?tree=Try&rev=ee5f8dc796bc

(Red platforms in the PGO run don't seem to build with PGO turned on. This is unrelated to the patches here.)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 119 • 11 years ago

Comment on attachment 796655 [details] [diff] [review]
Part 1: imglib patch

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

::: image/src/ImageEncoder.cpp
@@ +321,5 @@
> +/* static */
> +nsresult
> +ImageEncoder::GetImageEncoders(const nsAString& aType,
> +                               imgIEncoder** aEncoder,
> +                               imgIEncoder** aFallbackEncoder)

Why did you choose this approach of always creating two encoders? Wouldn't it make more sense to just return one encoder, and provide a method on imgIEncoder to determine what the type is?

::: image/src/ImageEncoder.h
@@ +26,5 @@
> +public:
> +  static nsresult ExtractData(const nsAString& aType,
> +                              const nsAString& aOptions,
> +                              const nsIntSize aSize,
> +                              nsICanvasRenderingContextInternal* aContext,

Having imglib depend on canvas in this way is rather ugly. Remind me why we decided to put this code in imglib?

@@ +28,5 @@
> +                              const nsAString& aOptions,
> +                              const nsIntSize aSize,
> +                              nsICanvasRenderingContextInternal* aContext,
> +                              nsIInputStream** aStream,
> +                              bool& aFellBackToPNG);

Instead of aFellBackToPNG, why not just make aType read/write so we can return the actual type we chose?

The methods in this class need documentation comments!

	Stephen A Pohl [:spohl] Assignee
	Comment 120 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #119)
> Comment on attachment 796655 [details] [diff] [review]
> Part 1: imglib patch
> 
> Review of attachment 796655 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/ImageEncoder.cpp
> @@ +321,5 @@
> > +/* static */
> > +nsresult
> > +ImageEncoder::GetImageEncoders(const nsAString& aType,
> > +                               imgIEncoder** aEncoder,
> > +                               imgIEncoder** aFallbackEncoder)
> 
> Why did you choose this approach of always creating two encoders? Wouldn't
> it make more sense to just return one encoder, and provide a method on
> imgIEncoder to determine what the type is?

This approach allows us to create all necessary imgIEncoder objects on the main thread and then hand them off to the encoder thread (in the toBlob case). This isn't strictly necessary in the toDataURL case yet, since it isn't asynchronous yet. But in the interest of simplifying our ExtractDataInternal function (and since we may want to make toDataURL asynchronous in the future), I thought it wouldn't hurt to consolidate this already. It is possible that aEncoder and aFallbackEncoder have the same type, but this redundancy already existed in the HTMLCanvasElement::ExtractData method. I hope this makes sense.


> ::: image/src/ImageEncoder.h
> @@ +26,5 @@
> > +public:
> > +  static nsresult ExtractData(const nsAString& aType,
> > +                              const nsAString& aOptions,
> > +                              const nsIntSize aSize,
> > +                              nsICanvasRenderingContextInternal* aContext,
> 
> Having imglib depend on canvas in this way is rather ugly. Remind me why we
> decided to put this code in imglib?

Sure: This came out of the feedback at the top of comment 77 and we discussed it in comment 92 to comment 95. If this no longer belongs here, would you know a good place to put it? Should this go in HTMLCanvasElement?


> @@ +28,5 @@
> > +                              const nsAString& aOptions,
> > +                              const nsIntSize aSize,
> > +                              nsICanvasRenderingContextInternal* aContext,
> > +                              nsIInputStream** aStream,
> > +                              bool& aFellBackToPNG);
> 
> Instead of aFellBackToPNG, why not just make aType read/write so we can
> return the actual type we chose?

I was trying to match the prototype of the existing HTMLCanvasElement::ExtractData method as closely as possible, but I can change this if you'd like. I'd have to change the callers a bit, but this wouldn't be a problem.


> The methods in this class need documentation comments!

True! I'll add them in my next patch once I hear back about the points above.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 121 • 11 years ago

(In reply to Stephen Pohl [:spohl] from comment #120)
> This approach allows us to create all necessary imgIEncoder objects on the
> main thread and then hand them off to the encoder thread (in the toBlob
> case). This isn't strictly necessary in the toDataURL case yet, since it
> isn't asynchronous yet. But in the interest of simplifying our
> ExtractDataInternal function (and since we may want to make toDataURL
> asynchronous in the future), I thought it wouldn't hurt to consolidate this
> already. It is possible that aEncoder and aFallbackEncoder have the same
> type, but this redundancy already existed in the
> HTMLCanvasElement::ExtractData method. I hope this makes sense.

Not yet :-). Why can't GetImageEncoders just return a single encoder, the right one?

> > ::: image/src/ImageEncoder.h
> > @@ +26,5 @@
> > > +public:
> > > +  static nsresult ExtractData(const nsAString& aType,
> > > +                              const nsAString& aOptions,
> > > +                              const nsIntSize aSize,
> > > +                              nsICanvasRenderingContextInternal* aContext,
> > 
> > Having imglib depend on canvas in this way is rather ugly. Remind me why we
> > decided to put this code in imglib?
> 
> Sure: This came out of the feedback at the top of comment 77 and we
> discussed it in comment 92 to comment 95. If this no longer belongs here,
> would you know a good place to put it? Should this go in HTMLCanvasElement?

Put it in a new file, say CanvasImageEncoding?

> > @@ +28,5 @@
> > > +                              const nsAString& aOptions,
> > > +                              const nsIntSize aSize,
> > > +                              nsICanvasRenderingContextInternal* aContext,
> > > +                              nsIInputStream** aStream,
> > > +                              bool& aFellBackToPNG);
> > 
> > Instead of aFellBackToPNG, why not just make aType read/write so we can
> > return the actual type we chose?
> 
> I was trying to match the prototype of the existing
> HTMLCanvasElement::ExtractData method as closely as possible, but I can
> change this if you'd like. I'd have to change the callers a bit, but this
> wouldn't be a problem.

Please.

Thanks!

	Stephen A Pohl [:spohl] Assignee
	Comment 122 • 11 years ago

Addressed feedback.

Moved ImageEncoder{.h|.cpp} to content/canvas/src/. I didn't rename the files to CanvasImageEncoder because they are used in both the CanvasRenderingContext2D and WebGLContext case.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #121)
> (In reply to Stephen Pohl [:spohl] from comment #120)
> > This approach allows us to create all necessary imgIEncoder objects on the
> > main thread and then hand them off to the encoder thread (in the toBlob
> > case). This isn't strictly necessary in the toDataURL case yet, since it
> > isn't asynchronous yet. But in the interest of simplifying our
> > ExtractDataInternal function (and since we may want to make toDataURL
> > asynchronous in the future), I thought it wouldn't hurt to consolidate this
> > already. It is possible that aEncoder and aFallbackEncoder have the same
> > type, but this redundancy already existed in the
> > HTMLCanvasElement::ExtractData method. I hope this makes sense.
> 
> Not yet :-). Why can't GetImageEncoders just return a single encoder, the
> right one?

It can, we would just need to call it twice in order to obtain the fallback encoder. I've done this here in case we want to go this route.

While I was at it, I've also removed the redundancy when the requested encoder type matched the fallback type (image/png).

	Stephen A Pohl [:spohl] Assignee
	Comment 123 • 11 years ago

Forgot to change aType in ExtractData when we fell back to "image/png".

	Stephen A Pohl [:spohl] Assignee
	Comment 124 • 11 years ago

Updated to work with the new patch for part 1. Setting this to r? as well.

Please refer to comment 117 for previous changes and especially the question at the bottom of said comment. Thanks!

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 125 • 11 years ago

Comment on attachment 799104 [details] [diff] [review]
Part 1: ImageEncoder class

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

::: content/canvas/src/ImageEncoder.cpp
@@ +297,5 @@
> +        imgStream = do_QueryInterface(currentEncoder);
> +      }
> +    }
> +
> +    if (NS_FAILED(rv) && currentEncoder != aPNGEncoder) {

Do you have any idea how currentEncoder could fail to encode, forcing us to fall back to aPNGEncoder?

I can understand falling back to PNG if the requested encoder doesn't exist. I don't know why an encoder would fail in a way where it would make sense to fall back to PNG.

::: content/canvas/src/ImageEncoder.h
@@ +37,5 @@
> +  // Extracts data asynchronously. A return value of NS_OK only implies
> +  // successful dispatching of the extraction step to the encoding thread.
> +  static nsresult ExtractDataAsync(const nsAString& aType,
> +                                   const nsAString& aOptions,
> +                                   bool aUsingCustomOptions,

Document this parameter and aOptions

	Stephen A Pohl [:spohl] Assignee
	Comment 126 • 11 years ago

Addressed feedback.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #125)
> Comment on attachment 799104 [details] [diff] [review]
> Part 1: ImageEncoder class
> 
> Review of attachment 799104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/ImageEncoder.cpp
> @@ +297,5 @@
> > +        imgStream = do_QueryInterface(currentEncoder);
> > +      }
> > +    }
> > +
> > +    if (NS_FAILED(rv) && currentEncoder != aPNGEncoder) {
> 
> Do you have any idea how currentEncoder could fail to encode, forcing us to
> fall back to aPNGEncoder?
> 
> I can understand falling back to PNG if the requested encoder doesn't exist.
> I don't know why an encoder would fail in a way where it would make sense to
> fall back to PNG.

No, I don't know how currentEncoder could fail. I just maintained existing behavior. The comment here referring to bug 329026 dates back to CVS, but I now realize that bug 329026 just asks for better DOM error messages in general. Changeset 42e56a8bb77a [1] for bug 401788 added code to explicitly fall back to image/png when an error was encountered here. I've removed the while loop and the PNG fallback on encoding error in this patch. I've maintained the fallback in case aEncoder is NULL. Running the test case [2] from bug 401788 shows that we continue to behave as expected, so this patch might actually simplify things while maintaining expected behavior.

[1] http://hg.mozilla.org/mozilla-central/rev/42e56a8bb77a
[2] http://philip.html5.org/tests/canvas/misc/exceptions.html

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 127 • 11 years ago

Comment on attachment 799723 [details] [diff] [review]
Part 1: ImageEncoder class

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

::: content/canvas/src/ImageEncoder.cpp
@@ +248,5 @@
> +  nsCOMPtr<nsIInputStream> imgStream;
> +  nsCOMPtr<imgIEncoder> currentEncoder(aEncoder);
> +  if (!currentEncoder) {
> +    aType.AssignLiteral("image/png");
> +    currentEncoder = aPNGEncoder;

Why does ExtractDataInternal need two encoders now? We can make ImageEncoder::GetImageEncoder take a mutable type parameter and return the PNG encoder if we don't have an encoder for the requested type. Then we always have just one encoder to work with.

	Stephen A Pohl [:spohl] Assignee
	Comment 128 • 11 years ago

Updated for current trunk. Carrying over r+.

	Stephen A Pohl [:spohl] Assignee
	Comment 129 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #127)
> Then we always have just one encoder to work with.

Nice! :-) Thanks!

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 130 • 11 years ago

Comment on attachment 799874 [details] [diff] [review]
Part 1: ImageEncoder class

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

::: content/canvas/src/ImageEncoder.cpp
@@ +54,5 @@
> +  uint64_t mImgSize;
> +  nsAutoString mType;
> +  JSContext* mJSContext;
> +  nsCOMPtr<nsIThread> mEncoderThread;
> +  nsCOMPtr<nsIFileCallback> mCallback;

Reorder these so the 64-bit field is first

@@ +136,5 @@
> +  int32_t mFormat;
> +  const nsIntSize mSize;
> +  nsCOMPtr<imgIEncoder> mEncoder;
> +  nsCOMPtr<nsIThread> mOriginThread;
> +  nsRefPtr<EncodingCompleteEvent> mEncodingCompleteEvent;

Reorder these: nsAutoStrings first, then pointers, then 32-bit values and nsIntSize, then bools

@@ +147,5 @@
> +ImageEncoder::ExtractData(nsAString& aType,
> +                          const nsAString& aOptions,
> +                          const nsIntSize aSize,
> +                          nsICanvasRenderingContextInternal* aContext,
> +                          nsIInputStream** aStream)

return already_AddRefed<nsIInputStream> instead of using an out parameter

@@ +225,5 @@
> +                                  uint8_t* aImageBuffer,
> +                                  int32_t aFormat,
> +                                  const nsIntSize aSize,
> +                                  nsICanvasRenderingContextInternal* aContext,
> +                                  nsIInputStream** aStream,

return already_AddRefed<nsIInputStream> instead of nsresult

@@ +277,5 @@
> +
> +/* static */
> +nsresult
> +ImageEncoder::GetImageEncoder(nsAString& aType,
> +                              imgIEncoder** aEncoder)

return already_AddRefed<imgIEncoder> instead of using an outparam

::: content/canvas/src/ImageEncoder.h
@@ +8,5 @@
> +
> +#include "imgIEncoder.h"
> +#include "nsDOMFile.h"
> +#include "nsError.h"
> +#include "nsIDOMHTMLCanvasElement.h"

This #include is not needed I think?

	Stephen A Pohl [:spohl] Assignee
	Comment 131 • 11 years ago

Thanks Rob! Addressed most of your feedback. Setting to r? again due to questions below:

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #130)
> @@ +147,5 @@
> > +ImageEncoder::ExtractData(nsAString& aType,
> > +                          const nsAString& aOptions,
> > +                          const nsIntSize aSize,
> > +                          nsICanvasRenderingContextInternal* aContext,
> > +                          nsIInputStream** aStream)
> 
> return already_AddRefed<nsIInputStream> instead of using an out parameter

The nsresult allowed the caller to differentiate between NS_ERROR_INVALID_ARG and any other error (such as NS_IMAGELIB_ERROR_NO_ENCODER for example). In the case of NS_ERROR_INVALID_ARG, the caller was encouraged to call this function again without any options. How can we differentiate between these errors when we return already_AddRefed<nsIInputStream> instead of the nsresult?

> @@ +225,5 @@
> > +                                  uint8_t* aImageBuffer,
> > +                                  int32_t aFormat,
> > +                                  const nsIntSize aSize,
> > +                                  nsICanvasRenderingContextInternal* aContext,
> > +                                  nsIInputStream** aStream,
> 
> return already_AddRefed<nsIInputStream> instead of nsresult

Same question as above, since this is called from ImageEncoder::ExtractData (as well as ImageEncoder::ExtractDataAsync).

> @@ +277,5 @@
> > +
> > +/* static */
> > +nsresult
> > +ImageEncoder::GetImageEncoder(nsAString& aType,
> > +                              imgIEncoder** aEncoder)
> 
> return already_AddRefed<imgIEncoder> instead of using an outparam

In the case of NS_IMAGELIB_ERROR_NO_ENCODER, would we simply return NULL? Would the caller need to know that this means NS_IMAGELIB_ERROR_NO_ENCODER and proceed accordingly? I had this logic in this function so that the caller can simply abort if nsresult wasn't equivalent to NS_OK and I thought that debugging with an error value might be easier, but I'm happy to change this. What should we set aType to if we return NULL (currently it would be set to "image/png", the fallback encoder)?

> ::: content/canvas/src/ImageEncoder.h
> @@ +8,5 @@
> > +
> > +#include "imgIEncoder.h"
> > +#include "nsDOMFile.h"
> > +#include "nsError.h"
> > +#include "nsIDOMHTMLCanvasElement.h"
> 
> This #include is not needed I think?

This is needed for the nsIFileCallback* parameter in ImageEncoder::ExtractDataAsync.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 132 • 11 years ago

(In reply to Stephen Pohl [:spohl] from comment #131)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #130)
> > @@ +147,5 @@
> > > +ImageEncoder::ExtractData(nsAString& aType,
> > > +                          const nsAString& aOptions,
> > > +                          const nsIntSize aSize,
> > > +                          nsICanvasRenderingContextInternal* aContext,
> > > +                          nsIInputStream** aStream)
> > 
> > return already_AddRefed<nsIInputStream> instead of using an out parameter
> 
> The nsresult allowed the caller to differentiate between
> NS_ERROR_INVALID_ARG and any other error (such as
> NS_IMAGELIB_ERROR_NO_ENCODER for example). In the case of
> NS_ERROR_INVALID_ARG, the caller was encouraged to call this function again
> without any options. How can we differentiate between these errors when we
> return already_AddRefed<nsIInputStream> instead of the nsresult?

Alright, never mind.

> 
> > @@ +225,5 @@
> > > +                                  uint8_t* aImageBuffer,
> > > +                                  int32_t aFormat,
> > > +                                  const nsIntSize aSize,
> > > +                                  nsICanvasRenderingContextInternal* aContext,
> > > +                                  nsIInputStream** aStream,
> > 
> > return already_AddRefed<nsIInputStream> instead of nsresult
> 
> Same question as above, since this is called from ImageEncoder::ExtractData
> (as well as ImageEncoder::ExtractDataAsync).

OK.

> > @@ +277,5 @@
> > > +
> > > +/* static */
> > > +nsresult
> > > +ImageEncoder::GetImageEncoder(nsAString& aType,
> > > +                              imgIEncoder** aEncoder)
> > 
> > return already_AddRefed<imgIEncoder> instead of using an outparam
> 
> In the case of NS_IMAGELIB_ERROR_NO_ENCODER, would we simply return NULL?
> Would the caller need to know that this means NS_IMAGELIB_ERROR_NO_ENCODER
> and proceed accordingly?

Yes.

> I had this logic in this function so that the
> caller can simply abort if nsresult wasn't equivalent to NS_OK and I thought
> that debugging with an error value might be easier, but I'm happy to change
> this. What should we set aType to if we return NULL (currently it would be
> set to "image/png", the fallback encoder)?

Say that's undefined.

> > ::: content/canvas/src/ImageEncoder.h
> > @@ +8,5 @@
> > > +
> > > +#include "imgIEncoder.h"
> > > +#include "nsDOMFile.h"
> > > +#include "nsError.h"
> > > +#include "nsIDOMHTMLCanvasElement.h"
> > 
> > This #include is not needed I think?
> 
> This is needed for the nsIFileCallback* parameter in
> ImageEncoder::ExtractDataAsync.

Hmm. OK.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 133 • 11 years ago

Comment on attachment 800231 [details] [diff] [review]
Part 1: ImageEncoder class

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

It's OK as-is I guess. You could change the return value of GetImageEncoder but it doesn't matter much.

	Stephen A Pohl [:spohl] Assignee
	Comment 134 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #133)
> It's OK as-is I guess. You could change the return value of GetImageEncoder
> but it doesn't matter much.

Great, thanks Rob! I'll make the change to ImageEncoder::GetImageEncoder and carry over the r+.

Try continues to be green for both regular and PGO builds. This didn't include the minor change to ImageEncoder::GetImageEncoder yet, but I don't expect that to change anything.

Regular try run: https://tbpl.mozilla.org/?tree=Try&rev=15c470c7aa69
PGO try run: https://tbpl.mozilla.org/?tree=Try&rev=638d7487300e

	Stephen A Pohl [:spohl] Assignee
	Comment 135 • 11 years ago

Addressed feedback regarding ImageEncoder::GetImageEncoder. Carrying over r+.

	Stephen A Pohl [:spohl] Assignee
	Comment 136 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/194d051724a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/db43e949245c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a59753a4cb13
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f0f85aea756
https://hg.mozilla.org/integration/mozilla-inbound/rev/89e680f30d31

	Stephen A Pohl [:spohl] Assignee
	Comment 137 • 11 years ago

Forgot to mention: Try run continues to look green after the last change to part 1.

https://tbpl.mozilla.org/?tree=Try&rev=874c7e643ac2

	Ryan VanderMeulen [:RyanVM]
	Comment 138 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/194d051724a8
https://hg.mozilla.org/mozilla-central/rev/db43e949245c
https://hg.mozilla.org/mozilla-central/rev/a59753a4cb13
https://hg.mozilla.org/mozilla-central/rev/9f0f85aea756
https://hg.mozilla.org/mozilla-central/rev/89e680f30d31

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 139 • 11 years ago

\o/

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 140 • 11 years ago

Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e8a4e466bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/3931b9652326

Please make sure to get someone who knows about JSAPI to review.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 141 • 11 years ago

In particular, someone who would notice that putting a JSContext* in a nsRunnable is a recipe for dangling pointers.

	Stephen A Pohl [:spohl] Assignee
	Comment 142 • 11 years ago

Comment on attachment 800824 [details] [diff] [review]
Part 1: ImageEncoder class

Setting back to r? per comment 140 and comment 141. :bz, please feel free to reassign if needed.

The question of JSContext* was discussed in comment 44.

	Stephen A Pohl [:spohl] Assignee
	Comment 143 • 11 years ago

Comment on attachment 799871 [details] [diff] [review]
Part 2: canvas patch

Setting back to r? per comment 140 and comment 141. :bz, please feel free to reassign if needed.

	Boris Zbarsky [:bzbarsky]
	Comment 144 • 11 years ago

> Just make sure that this code doesn't run after the element is destroyed and the callback
> has been revoked.

Where does this part happen?

But also, it's not obvious to me that even ensuring that is enough...

	Stephen A Pohl [:spohl] Assignee
	Comment 145 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #144)
> > Just make sure that this code doesn't run after the element is destroyed and the callback
> > has been revoked.
> 
> Where does this part happen?
> 
> But also, it's not obvious to me that even ensuring that is enough...

Hmm, I guess this isn't enforced right now. Would you have a suggestion how to do this properly? Ideally, there would be a way to get the JSContext that toBlob was called on from the encoding thread...

	Boris Zbarsky [:bzbarsky]
	Comment 146 • 11 years ago

You could store a strong ref to an nsIScriptContext instead, and get the JSContext from it as needed?

Just make sure to assert mainthread for any code touching the JSContext.

	Ryan VanderMeulen [:RyanVM]
	Comment 147 • 11 years ago

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a5e8a4e466bc

	Ryan VanderMeulen [:RyanVM]
	Comment 148 • 11 years ago

Dammit, meant to uncheck the "Resolve" button.

	Stephen A Pohl [:spohl] Assignee
	Comment 149 • 11 years ago

Just asking for feedback at this point.

This patch stores an nsIScriptContext and we get the JSContext from it when the encoding is complete.

(In reply to Boris Zbarsky [:bz] from comment #146)
> Just make sure to assert mainthread for any code touching the JSContext.

I'm not sure I fully understand this. The JSContext is null-checked before use, but I'm not sure that covers what you meant here.

	Stephen A Pohl [:spohl] Assignee
	Comment 150 • 11 years ago

This patch gets the nsIScriptContext via GetScriptContextFromJSContext and passes it to ExtractDataAsync. It'd be great to know if this is the proper way to get the nsIScriptContext in this situation.

	Boris Zbarsky [:bzbarsky]
	Comment 151 • 11 years ago

Comment on attachment 801787 [details] [diff] [review]
Part 1: ImageEncoder class

I meant that EncodingCompleteEvent::Run needs to assert NS_IsMainThread().

>+    JSContext* jsContext = mScriptContext->GetNativeContext();

This probably needs to handle null mScriptContext too.

	Boris Zbarsky [:bzbarsky]
	Comment 152 • 11 years ago

Comment on attachment 801789 [details] [diff] [review]
Part 2: canvas patch

That's the right way to do it, yes.  Might return null, of course.

	Stephen A Pohl [:spohl] Assignee
	Comment 153 • 11 years ago

Just updated for current trunk, no other changes. Setting to r?.

	Stephen A Pohl [:spohl] Assignee
	Comment 154 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #151)
> I meant that EncodingCompleteEvent::Run needs to assert NS_IsMainThread().

Hmm, previous iterations of my patches failed when I assumed that interactions occurred between the main thread and encoder thread only. At least on Linux, I used to run into try failures that seemed to insinuate that the call to toBlob occurred on a non-main thread. To be honest, it didn't make sense to me at the time but once I started tracking the origin thread of the toBlob call, the failures disappeared. I've asserted NS_IsMainThread() in EncodingCompleteEvent::Run and ran the change through try, expecting the same failures. To my surprise, no failures were present anymore.

This means that we should be able to remove the code that retrieved the current thread in HTMLCanvasElement::ToBlob and the mOriginThread member in the EncodingRunnable class. Instead, we simply dispatch the EncodingCompleteEvent directly to the main thread.

Try is looking green so far:
https://tbpl.mozilla.org/?tree=Try&rev=0aadc30a1cff

Setting to r?.

	Boris Zbarsky [:bzbarsky]
	Comment 155 • 11 years ago

> To my surprise, no failures were present anymore.

Good, because doing anything with an nsIScriptContext not on the main thread would be really bad. ;)