Closed Bug 685516 Opened 13 years ago Closed 12 years ago

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

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: khuey, Assigned: khuey)

References

(Blocks 1 open bug)

Details

(Whiteboard: [snappy:P1])

Attachments

(2 files)

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.
Attached patch PatchSplinter Review
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
Closed: 13 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
Blocks: image-suck
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]
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
Depends on: 783182
Depends on: 651866
Depends on: 769975
Some of these disabled tests might have caught bug 783355 ...
Depends on: 783466
Depends on: 783748
No longer depends on: 783748
Depends on: 783748
Blocks: 784756
Depends on: 786357
Blocks: 786357
No longer depends on: 786357
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.