This doesn't happen in the nightly from the 20th of march. I see this on this page: http://code.google.com/p/webrtc/

This may have gotten better in some cases, but it's not fixed yet.

In the new world, we've inserted an extra event loop go-around (or two) because we only RequestDecode, which never does any main thread decoding. This patch, which I'd like to apply to Aurora, simply changes us to do synchronous decoding when we switch tabs. This is OK because we now only track images that are visible.

This is the riskier but more exciting patch. The previous patch is safer, but it might cause some main-thread jank, and it doesn't make use of multithreaded decoding at all when we switch tabs. When we switch tabs, we RequestDecode on the tab's visible images. This kicks off multithreaded decoding of those images; usually, these decodes will complete very, very quickly, and fully asynchronously to the main thread. These decodes will send the main thread update messages, and if we do nothing else, they'll be put at the end of the event queue, which means we draw the image as empty until the update message (FinishedSomeDecoding) gets processed and the image is invalidated. With this patch, when we're drawing (and doing anything else that calls StartDecoding, including WantDecodedFrames()), we'll explicitly call FinishedSomeDecoding if it's needed; this will ensure that we have all the image's attributes and metadata without going through the event loop. (Doing this is already transparently handled, since we frequently do things like SyncDecode() on an image that has otherwise been queued up for decoding or decoded already.)

attachment 741854 [details] [diff] [review] pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=dfb57e67e3f0

attachment 741518 [details] [diff] [review] pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ac9238fa920e

Comment on attachment 741518 [details] [diff] [review] StartDecoding() on tab switch Review of attachment 741518 [details] [diff] [review]: ----------------------------------------------------------------- You should make sure you include information in the commit message about how this is worse than the pre-multi-threaded world because now we're decoding some nearby images that are not being painted.

This should track, almost certainly. It's a pretty visible regression.

Previous patch hung if we hit animated images. (Told you it was riskier!) This patch fixes that. https://tbpl.mozilla.org/?tree=Try&rev=9d1daf69061d

Comment on attachment 741962 [details] [diff] [review] Call FinishedSomeDecoding() if we're being asked to do a somewhat synchronous decode Review of attachment 741962 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/src/RasterImage.cpp @@ +2743,5 @@ > > + // If we're already decoded, there's nothing to do. This check is explicitly > + // duplicated to avoid the extra overhead of this function in the > + // already-decoded case, because it's called a lot. > + if (mDecoded) IMO these comments about the duplication of this check focus on the wrong thing. The natural location for this check is here, at the top of the function, and the reason you need a second check later is because (as your comment there notes) the state of mDecoded can change. If it was me writing these comments, I'd just focus on that. @@ +2755,5 @@ > > + // If we're currently waiting for a new frame, we can't do anything until > + // that frame is allocated. > + if (mDecoder && mDecoder->NeedsNewFrame()) > + return NS_OK; Do you care that the value you get from NeedsNewFrame could change out from under you? (i.e., does later code in this method implicitly assume that NeedsNewFrame is false?) You're reading this outside the lock. @@ +2781,5 @@ > return NS_OK; > } > } > > + MutexAutoLock lock(mDecodingMutex); What sucks about taking this lock here is that, because mDecodingMutex is held for the entire time a decoder is decoding, it is usually the case that calling RequestDecodeCore multiple times is equivalent to running a synchronous decode. I know this is nothing new, though. @@ +2788,5 @@ > + // asked to do a somewhat synchronous decode, go ahead and do that. > + if (aDecodeType == SOMEWHAT_SYNCHRONOUS && mDecodeRequest && > + mDecodeRequest->mRequestStatus == DecodeRequest::REQUEST_WORK_DONE) { > + FinishedSomeDecoding(); > + } Why is the |aDecodeType == SOMEWHAT_SYNCHRONOUS| test here beneficial? On m-c you were just notifying whenever |mRequestStatus == DecodeRequest::REQUEST_WORK_DONE|, which seems better to me.

(In reply to Seth Fowler [:seth] from comment #10) > > + // If we're currently waiting for a new frame, we can't do anything until > > + // that frame is allocated. > > + if (mDecoder && mDecoder->NeedsNewFrame()) > > + return NS_OK; > > Do you care that the value you get from NeedsNewFrame could change out from > under you? (i.e., does later code in this method implicitly assume that > NeedsNewFrame is false?) You're reading this outside the lock. Hm, yes. This can be called synchronously from FrameNeededWorker, which this is guarding against. Other than that we're fine, I think. (*Think.*) > What sucks about taking this lock here is that, because mDecodingMutex is > held for the entire time a decoder is decoding, it is usually the case that > calling RequestDecodeCore multiple times is equivalent to running a > synchronous decode. I know this is nothing new, though. The check for mBytesDecoded should make this not the case, no? > Why is the |aDecodeType == SOMEWHAT_SYNCHRONOUS| test here beneficial? I thought about this. I'm willing to listen to reason here, but the SOMEWHAT_SYNCHRONOUS check makes it so that we never do any drawing/notification synchronously unless we've been asked to. So it should make us a little more responsive. I don't feel strongly about this, though - if you do please convince me further! > On > m-c you were just notifying whenever |mRequestStatus == > DecodeRequest::REQUEST_WORK_DONE|, which seems better to me. I don't understand this bit; we never notified from RequestDecode prior to this patch.

(In reply to Joe Drew (:JOEDREW! \o/) from comment #11) > I don't understand this bit; we never notified from RequestDecode prior to > this patch. ??? https://mxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#2852

(In reply to Joe Drew (:JOEDREW! \o/) from comment #11) > > What sucks about taking this lock here is that, because mDecodingMutex is > > held for the entire time a decoder is decoding, it is usually the case that > > calling RequestDecodeCore multiple times is equivalent to running a > > synchronous decode. I know this is nothing new, though. > > The check for mBytesDecoded should make this not the case, no? Ah, yes. It can still happen, though, because reading mBytesDecoded may yield 0 when we perform the initial check, but we may still find later when we try to take the lock that it's busy. I take back usually; it's not clear how often it will actually happen.

(In reply to Joe Drew (:JOEDREW! \o/) from comment #11) > > Why is the |aDecodeType == SOMEWHAT_SYNCHRONOUS| test here beneficial? > > I thought about this. I'm willing to listen to reason here, but the > SOMEWHAT_SYNCHRONOUS check makes it so that we never do any > drawing/notification synchronously unless we've been asked to. So it should > make us a little more responsive. If it's pretty cheap to notify it seems like it should always be a win to do it (less total work since we don't have to invalidate things later when we could've painted the image right away, other work that depends on the image can start sooner, that sort of thing). If there are callers that couldn't handle synchronous notification at this point, though, that might be a good reason not to introduce it.

(In reply to Seth Fowler [:seth] from comment #12) > (In reply to Joe Drew (:JOEDREW! \o/) from comment #11) > > I don't understand this bit; we never notified from RequestDecode prior to > > this patch. > > ??? > > https://mxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#2852 Dude, that's in SyncDecode.

(In reply to Seth Fowler [:seth] from comment #14) > If there are callers that couldn't > handle synchronous notification at this point, though, that might be a good > reason not to introduce it. This definitely shouldn't be the case, since before bug 716140 almost all notifications could have synchronous notifications.

(In reply to Joe Drew (:JOEDREW! \o/) from comment #15) > (In reply to Seth Fowler [:seth] from comment #12) > > (In reply to Joe Drew (:JOEDREW! \o/) from comment #11) > > > I don't understand this bit; we never notified from RequestDecode prior to > > > this patch. > > > > ??? > > > > https://mxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#2852 > > Dude, that's in SyncDecode. Sorry, I copied the wrong link somehow. https://mxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#2803

It looks like RasterImage::GetCurrentImage() depends on RequestDecode notifying asynchronously, though I'm not familiar with the exact reason why. https://mxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#1064

(In reply to Seth Fowler [:seth] from comment #17) > (In reply to Joe Drew (:JOEDREW! \o/) from comment #15) > > (In reply to Seth Fowler [:seth] from comment #12) > > > (In reply to Joe Drew (:JOEDREW! \o/) from comment #11) > > > > I don't understand this bit; we never notified from RequestDecode prior to > > > > this patch. > > > > > > ??? > > > > > > https://mxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#2852 > > > > Dude, that's in SyncDecode. > > Sorry, I copied the wrong link somehow. > > https://mxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#2803 Good lord. What is wrong with me.

Comment on attachment 741518 [details] [diff] [review] StartDecoding() on tab switch [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 716140 User impact if declined: Flashing on tab switch Testing completed (on m-c, etc.): locally Risk to taking this patch (and alternatives if risky): Patch itself is very safe. Biggest risk is that we'll introduce some jank. The mozilla-central patch, which I've just checked in, should alleviate all that jank, but is much riskier. String or IDL/UUID changes made by this patch: none

Note that this isn't fully fixed: bug 869125.

I tried to reproduce this issue on a Nightly build from 04/01 opened in 32bit mode on Mac OSX 10.7.5, but didn't manage to. If anyone can reproduce this issue, please try to verify it.

Jeff, since you were able to reproduce this earlier can you please verify it's fixed in Firefox 23 Beta?

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #26) > Jeff, since you were able to reproduce this earlier can you please verify > it's fixed in Firefox 23 Beta? Canceling this long overdue request.