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)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: khuey, Assigned: khuey)
References
(Blocks 1 open bug)
Details
(Whiteboard: [snappy:P1])
Attachments
(2 files)
3.08 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
46.03 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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•13 years ago
|
||
Fwiw, this sounds like a great solution to me in terms of UX.
Assignee | ||
Comment 3•13 years ago
|
||
Comment 4•13 years ago
|
||
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 5•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/367ff8c94636
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Assignee | ||
Comment 7•13 years ago
|
||
(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•13 years ago
|
||
Backed out for Android reftest failures: https://hg.mozilla.org/mozilla-central/rev/29c1738d7e27
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla10 → ---
Assignee | ||
Comment 9•13 years ago
|
||
Per IRC conversation with joe, the root problem here is that drawWindow doesn't seem to cause sync decoding of CSS images.
Assignee | ||
Comment 10•13 years ago
|
||
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
Assignee | ||
Comment 11•13 years ago
|
||
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
Updated•13 years ago
|
Blocks: image-suck
Comment 12•12 years ago
|
||
Presumably we can now land this, as we don't care about multiprocess images.
Assignee | ||
Comment 13•12 years ago
|
||
B2G will care, and I'd rather not land stuff that's known to be broken.
Assignee | ||
Comment 14•12 years ago
|
||
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•12 years ago
|
||
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]
Assignee | ||
Comment 16•12 years ago
|
||
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 17•12 years ago
|
||
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+
Comment 18•12 years ago
|
||
Disabled background-size-body-percent-percent-overflow.html; rs=khuey https://hg.mozilla.org/mozilla-central/rev/9c67b2ff95b7
Assignee | ||
Comment 19•12 years ago
|
||
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
Closed: 13 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Some of these disabled tests might have caught bug 783355 ...
Comment 21•12 years ago
|
||
Mark more as random: https://hg.mozilla.org/mozilla-central/rev/f5f29adc6d30 (Bug 769975 & bug 783182)
Comment 22•12 years ago
|
||
Mark another as random: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb2bd7862bbd (Bug 783466)
Updated•12 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•