1051531 - asking for a decode of an image more than once will block on the decoding thread

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Timothy Nikkel (:tnikkel)]

If we call RasterImage::RequestDecodeCore while a decode is running on a decoding thread then we try to acquire the decoding mutex and the main thread will have to wait until the decoder yields. This is sad because it means we don't see the main benefit of off main thread image decoding.

Before bug 853564 we had a |if (mDecoder && !mDecoder->IsSizeDecode() && mBytesDecoded)| check before we tried to acquire the mutex that would usual make us early exit if a decoding thread was in progress. This was noted in the bug. But unfortunately this if had to be moved after a FinishedSomeDecoding call so that decodes would get completed without going back to the event loop. And this meant the check had to be moved inside the lock because FinishedSomeDecoding has to be done while holding the lock.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

I think we can just move this code outside of the lock. We of course have to lock when actually calling FinishedSomeDecoding, but the checks should be okay.

Review this carefully please.

Comment 2

[Timothy Nikkel (:tnikkel)]

green on tryserver

Comment 3

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8470474 [details] [diff] [review]
patch

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

::: image/src/RasterImage.cpp
@@ +2339,1 @@
>      nsresult rv = FinishedSomeDecoding();

Hmm, I'm not sure...

The problem is that mDecodeRequest->mRequestStatus might get set to DecodeRequest::REQUEST_WORK_DONE after we pass this point. The |if (mDecoder && !mDecoder->IsSizeDecode() && mBytesDecoded)| check later on may then cause us to return NS_OK without attempting to actually finish the decode. This means we won't behave as the caller may expect us to when they pass SYNCHRONOUS_NOTIFY or SYNCHRONOUS_NOTIFY_AND_SOME_DECODE.

So the question is basically: do we care about losing this invariant?

There's really no way to achieve your goal in this bug without losing it, as long as we keep the current locking protocol in place.

A better solution may be to think about changing the locking protocol. It may become necessary soon anyway. I'm not sure that the existing design is going to work once we have multiple simultaneous decoders, which is going to happen soon.

Comment 4

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] from comment #3)
> The problem is that mDecodeRequest->mRequestStatus might get set to
> DecodeRequest::REQUEST_WORK_DONE after we pass this point. The |if (mDecoder
> && !mDecoder->IsSizeDecode() && mBytesDecoded)| check later on may then
> cause us to return NS_OK without attempting to actually finish the decode.
> This means we won't behave as the caller may expect us to when they pass
> SYNCHRONOUS_NOTIFY or SYNCHRONOUS_NOTIFY_AND_SOME_DECODE.

I can't see any caller depending on this. If a caller were depending on RequestDecodeCore to notify if the status becomes REQUEST_WORK_DONE at any point while the main thread is in RequestDecodeCore then how can that caller deal with the situation where the status becomes REQUEST_WORK_DONE the instant after we return to the caller from RequestDecodeCore?

I think the only reasonable expectation for RequestDecodeCore is that it checks if the status is REQUEST_WORK_DONE at some point and notifies if it is.

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Timothy Nikkel (:tn) from comment #4)
> I can't see any caller depending on this.

Yeah, I agree. Looking at it again, it seems to me that the synchronous vs asynchronous distinction here is not about whether we *must* deliver notifications synchronously if possible, but about whether delivering notifications synchronously is an *option*.

Let's land this thing. Thanks, Timothy!

Comment 6

[Timothy Nikkel (:tnikkel)]

While thinking about your review comment (thanks!) I realized there are two problems with this patch:
1) if we call RequestDecodeCore with SYNCHRONOUS_NOTIFY_AND_SOME_DECODE and mBytesDecoded is > 0 then we will not wait for the lock (ie not wait for any decoding) and not do any decoding ourselves. I think we should always do at least one of those two things for SYNCHRONOUS_NOTIFY_AND_SOME_DECODE.
2) If we still have to acquire the lock after these early exits then there might be work done to notify at that point, and we should notify for it (if we are allowed to).

I'll get a new patch posted to fix these.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

I changed this to take into account comment 6.

Specifically: I made the mBytesDecoded early exit that occurs before getting the lock only happen if we weren't asked to do some decoding. I also WORK_DONE check to do FinishedSomeDecoding before getting the lock only happen for SYNCHRONOUS_NOTIFY (and not the other two kinds).

Comment 8

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8478693 [details] [diff] [review]
patch v2

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

This is what I was envisioning when I wrote my original review comments. I have to say that your objection that callers couldn't reasonably depend on this behavior was convincing to me! However, I think this is a good way to go too.

Comment 9

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a99f3b29c4a8

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a99f3b29c4a8