853564 - Small images flash on tab switch

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

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

Comment 2

[Joe Drew (not getting mail)]

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

Comment 3

[Joe Drew (not getting mail)]

Attachment: StartDecoding() on tab switch

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.

Comment 4

[Joe Drew (not getting mail)]

Attachment: Call FinishedSomeDecoding() if we're being asked to do a somewhat synchronous decode

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.)

Comment 5

[Joe Drew (not getting mail)]

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

Comment 6

[Joe Drew (not getting mail)]

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

Comment 7

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 8

[Joe Drew (not getting mail)]

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

Comment 9

[Joe Drew (not getting mail)]

Attachment: Call FinishedSomeDecoding() if we're being asked to do a somewhat synchronous decode

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 10

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

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.

Comment 11

[Joe Drew (not getting mail)]

(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.

Comment 12

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

(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

Comment 13

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

(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.

Comment 14

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

(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.

Comment 15

[Joe Drew (not getting mail)]

(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.

Comment 16

[Joe Drew (not getting mail)]

(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.

Comment 17

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

(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

Comment 18

[Robert Lickenbrock [:rclick]]

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

Comment 19

[Joe Drew (not getting mail)]

(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 20

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45d40d37b1df

Comment 21

[Joe Drew (not getting mail)]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45d40d37b1df

Comment 23

[Joe Drew (not getting mail)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/93ff2a7991fd

Comment 24

[Joe Drew (not getting mail)]

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

Comment 25

[Ioana (away)]

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.

Comment 26

[u279076]

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

Comment 27

[u279076]

(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.