Closed Bug 1051531 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8470474 - Flags: review?(seth)
green on tryserver
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.
(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.
(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!
Attachment #8470474 - Flags: review?(seth) → review+
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.
Attached patch patch v2Splinter Review
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).
Attachment #8470474 - Attachment is obsolete: true
Attachment #8478693 - Flags: review?(seth)
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.
Attachment #8478693 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/a99f3b29c4a8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.