The default bug view has changed. See this FAQ.

Mitigate flickering problems when inserting images into the DOM that have no decoded data

RESOLVED FIXED in mozilla17

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [snappy:P1])

Attachments

(2 attachments)

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.
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.
Fwiw, this sounds like a great solution to me in terms of UX.
Created attachment 560643 [details] [diff] [review]
Patch
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Attachment #560643 - Flags: review?(joe)
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."
Attachment #560643 - Flags: review?(joe) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/367ff8c94636
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(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.
Backed out for Android reftest failures:
https://hg.mozilla.org/mozilla-central/rev/29c1738d7e27
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla10 → ---
Per IRC conversation with joe, the root problem here is that drawWindow doesn't seem to cause sync decoding of CSS images.
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.
Assignee: khuey → nobody
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.
Assignee: nobody → khuey
Depends on: 695763
Depends on: 695765

Updated

6 years ago
Blocks: 683284
Depends on: 697230
Presumably we can now land this, as we don't care about multiprocess images.
B2G will care, and I'd rather not land stuff that's known to be broken.
Also, after thinking about this a bit more, if we land this as is we'll be landing a nice random orange timebomb.
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.)
Whiteboard: [snappy:P1]
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.
Attachment #644319 - Flags: review?(jmuizelaar)
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.
Attachment #644319 - Flags: review?(jmuizelaar) → review+
Disabled background-size-body-percent-percent-overflow.html; rs=khuey 

https://hg.mozilla.org/mozilla-central/rev/9c67b2ff95b7
https://hg.mozilla.org/mozilla-central/rev/3bb125057939

And test disablings:
https://hg.mozilla.org/mozilla-central/rev/d3829aea9cd1
https://hg.mozilla.org/mozilla-central/rev/b11ae0f50e1e
https://hg.mozilla.org/mozilla-central/rev/b9b3b3373c08
https://hg.mozilla.org/mozilla-central/rev/9c67b2ff95b7
https://hg.mozilla.org/mozilla-central/rev/131799806373
https://hg.mozilla.org/mozilla-central/rev/996a68d8420e
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 783182
Depends on: 651866
Depends on: 769975
Some of these disabled tests might have caught bug 783355 ...
Depends on: 783466
Mark more as random:
https://hg.mozilla.org/mozilla-central/rev/f5f29adc6d30

(Bug 769975 & bug 783182)

Updated

5 years ago
Depends on: 783748

Updated

5 years ago
No longer depends on: 783748
Mark another as random:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb2bd7862bbd

(Bug 783466)

Updated

5 years ago
Depends on: 783748
https://hg.mozilla.org/mozilla-central/rev/bb2bd7862bbd

Updated

5 years ago
Blocks: 784756
Depends on: 786357
Blocks: 786357
No longer depends on: 786357

Updated

5 years ago
Blocks: 787766
No longer depends on: 695763
Depends on: 879494
You need to log in before you can comment on or make changes to this bug.