Last Comment Bug 685516 - Mitigate flickering problems when inserting images into the DOM that have no decoded data
: Mitigate flickering problems when inserting images into the DOM that have no ...
Status: RESOLVED FIXED
[snappy:P1]
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: All All
: -- normal with 5 votes (vote)
: mozilla17
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
Mentors:
Depends on: 651866 695765 697230 769975 783182 783466 783748 879494
Blocks: image-suck 786357 683290 784756 787766
  Show dependency treegraph
 
Reported: 2011-09-08 06:01 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2013-06-04 14:17 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.08 KB, patch)
2011-09-16 15:58 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
joe: review+
Details | Diff | Splinter Review
Disable failing Android XUL tests (46.03 KB, patch)
2012-07-20 07:26 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
jmuizelaar: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-08 06:01:31 PDT
On IRC joe suggested doing this by changing RasterImage::RequestDecode from only doing a decode synchronously if the image is under some size to always decoding synchronously for $CONSTANT ms, and then posting the rest of the task to event loop if the decode did not finish in under $CONSTANT ms.

Making this change should be easy for the case where we're not already in a size decode, and I think we can probably get away with ignoring that case.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-08 06:38:21 PDT
The only part of this that seems hard to me is continuing to ensure that we satisfy consumers expectations (if there are any?) about async notifications.  Does SyncDecode notify asynchronously?  If not, this should be pretty straightforward, I think.
Comment 2 Boris Zbarsky [:bz] (TPAC) 2011-09-08 07:27:59 PDT
Fwiw, this sounds like a great solution to me in terms of UX.
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-16 15:58:25 PDT
Created attachment 560643 [details] [diff] [review]
Patch
Comment 4 Joe Drew (not getting mail) 2011-09-16 18:06:12 PDT
Comment on attachment 560643 [details] [diff] [review]
Patch

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

Remove the pref from all.js too, and make sure it's not used in mobile's default prefs.

::: modules/libpr0n/src/RasterImage.cpp
@@ +2388,5 @@
>    // If we've read all the data we have, we're done
>    if (mBytesDecoded == mSourceData.Length())
>      return NS_OK;
>  
> +  // If we can do decoding now, do so.  Small images will decode completely,

"If we're able to decode now, do so."
Comment 5 Robert Lickenbrock [:rclick] 2011-09-22 12:11:16 PDT
Comment on attachment 560643 [details] [diff] [review]
Patch

> +  if (!mDecoded && !mInDecoder && mHasSourceData)
> +    return mWorker->Run();

You should be able to call mWorker->Run() unconditionally here. We return early if we're fully decoded, called from inside the decoder, or don't have any data to decode. If we make it this far, our preconditions for decoding should already be true.
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-10 09:20:44 PDT
https://hg.mozilla.org/mozilla-central/rev/367ff8c94636
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-10 09:22:07 PDT
(In reply to Robert Lickenbrock [:rclickenbrock] from comment #5)
> Comment on attachment 560643 [details] [diff] [review] [diff] [details] [review]
> Patch
> 
> > +  if (!mDecoded && !mInDecoder && mHasSourceData)
> > +    return mWorker->Run();
> 
> You should be able to call mWorker->Run() unconditionally here. We return
> early if we're fully decoded, called from inside the decoder, or don't have
> any data to decode. If we make it this far, our preconditions for decoding
> should already be true.

Lets do that in another bug please.
Comment 8 Matt Brubeck (:mbrubeck) 2011-10-10 13:49:05 PDT
Backed out for Android reftest failures:
https://hg.mozilla.org/mozilla-central/rev/29c1738d7e27
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-11 08:45:15 PDT
Per IRC conversation with joe, the root problem here is that drawWindow doesn't seem to cause sync decoding of CSS images.
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-13 08:01:16 PDT
I've spent the last two days trying to get to a point where I can run reftests on android under a debugger without success, so I'm giving up here.
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-14 14:52:06 PDT
Ok, I finally managed to get this under a debugger.  The reftest harness is just broken for the multiprocess world.  Still tracking down exactly what's wrong.
Comment 12 Joe Drew (not getting mail) 2011-12-25 16:46:00 PST
Presumably we can now land this, as we don't care about multiprocess images.
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-25 18:12:47 PST
B2G will care, and I'd rather not land stuff that's known to be broken.
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-26 16:04:25 PST
Also, after thinking about this a bit more, if we land this as is we'll be landing a nice random orange timebomb.
Comment 15 Justin Lebar (not reading bugmail) 2012-02-14 16:23:22 PST
Marking as Snappy P1: Right now, if we have a bunch of near-150KB images on a page, when those images come in from cache or are discarded and re-decoded, we'll decode each image synchronously.  It's bad.  (Perhaps this should be P2; I'm not sure how often we hit this case.  But 150KB *compressed* is pretty big.)
Comment 16 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-20 07:26:16 PDT
Created attachment 644319 [details] [diff] [review]
Disable failing Android XUL tests

After several days of attempting to debug and reproduce, we've decided to disable the offending tests.  We couldn't reproduce this on desktop with reftest-ipc, or even on Android XUL on multiple phones/tegras.
Comment 17 Jeff Muizelaar [:jrmuizel] 2012-07-20 14:39:46 PDT
Comment on attachment 644319 [details] [diff] [review]
Disable failing Android XUL tests

I'd like this to be random-if(somethingBetterThanAndroidetc) so that we can revert all of these later if we stop testing this configuration.
Comment 18 Ed Morley [:emorley] 2012-08-15 09:23:35 PDT
Disabled background-size-body-percent-percent-overflow.html; rs=khuey 

https://hg.mozilla.org/mozilla-central/rev/9c67b2ff95b7
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-16 15:45:51 PDT
Some of these disabled tests might have caught bug 783355 ...
Comment 21 Ed Morley [:emorley] 2012-08-17 06:33:21 PDT
Mark more as random:
https://hg.mozilla.org/mozilla-central/rev/f5f29adc6d30

(Bug 769975 & bug 783182)
Comment 22 Ed Morley [:emorley] 2012-08-18 06:53:16 PDT
Mark another as random:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb2bd7862bbd

(Bug 783466)
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-08-18 16:19:18 PDT
https://hg.mozilla.org/mozilla-central/rev/bb2bd7862bbd

Note You need to log in before you can comment on or make changes to this bug.