Closed Bug 622470 Opened 9 years ago Closed 9 years ago

Decode on draw must be enabled for Mobile Fennec

Categories

(Core :: ImageLib, defect)

ARM
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: romaxa, Assigned: romaxa)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 9 obsolete files)

After investigation of fennec slow loading on test page from bug 622397, I found that we are wasting a lot of CPU/memory resources while loading and decoding images on page loading.

I think we must enable decode on draw and don't decode images which are offscreen, and use only thebes buffer as storage for page images
Blocks: 622475
joe, is this something we can trivially turn on?
that is about desktop, and on desktop we don't have offscreen thebes layer with already cached images...
Component: Graphics → ImageLib
QA Contact: thebes → imagelib
With enabled decodeondraw I see huge amount of IPC progress/onstatus messages eating all CPU... 
simple counte show me that we do call ~50-100 messages per second which endup in JS code... this looks really bad..
The only way get into direct write to decoder : http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/RasterImage.cpp#1181

is set:
pref("image.mem.discardable", false);
pref("image.mem.decodeondraw", false);

joe is StoringSourceData condition correct? should not be image.mem.decodeondraw = true ?
Even with 
pref("image.mem.discardable", false);
pref("image.mem.decodeondraw", false);

I still see imgFrame::Init and new gfxImageSurface allocation with this backtrace
How decode on draw feature suppose to work?
Depends on: 626419
Don't know who should review this patch.. 
But this will disable image lock/unlock for content process documents.

Images will be create on paint request with (image.mem.decodeondraw) and discarded by timeout after 10 seconds. (and available only on thebes layer)
Attachment #500782 - Attachment is obsolete: true
Attachment #500783 - Attachment is obsolete: true
Attachment #500785 - Attachment is obsolete: true
Attachment #500820 - Attachment is obsolete: true
Attachment #504682 - Flags: review?
Attachment #504682 - Flags: review? → review?(joe)
Attached patch image.mem.decodeondraw = true (obsolete) — Splinter Review
image.mem.decodeondraw = true,
Also I found some problem with async decoding, when async decoding finished nsImageFrame::OnAvailableData generate Invalidate(r), but that invalidate does not  repaint Remote fennec viewport...

For now I made all images <= 15Mb async decoded...
Blocks: 608385
Attached patch Update CSS viewport (obsolete) — Splinter Review
I found one reason of why async decode Invalidate request is not appearing on PuppetWidget and IPC layers... that is related to Invalidate and viewport position...
Attachment #504685 - Flags: review?
Comment on attachment 504685 [details] [diff] [review]
Update CSS viewport

Also broken CSS viewport breaking bottom page image animation (gif and others)
Attachment #504685 - Flags: review? → review?(jones.chris.g)
Comment on attachment 504685 [details] [diff] [review]
Update CSS viewport

Sorry, I can't review frontend code.  You probably want :stechz, :mbrubeck, :vingetun, or :mfinkle instead.
Attachment #504685 - Flags: review?(jones.chris.g)
Attachment #504685 - Flags: review?(21)
Comment on attachment 504685 [details] [diff] [review]
Update CSS viewport

This patch tries to do resolve bug 479862 basically. The proposed solution are pretty closed.
Comment on attachment 504685 [details] [diff] [review]
Update CSS viewport

Possibly this patch should go to https://bugzilla.mozilla.org/show_bug.cgi?id=479862 ...
Comment on attachment 504682 [details] [diff] [review]
Disable Tab active/inactive locking for content process

This looks OK to me, but I want Boris to take a look too since he reviewed this code when Bobby wrote it.
Attachment #504682 - Flags: review?(joe)
Attachment #504682 - Flags: review?(bzbarsky)
Attachment #504682 - Flags: review+
Comment on attachment 504682 [details] [diff] [review]
Disable Tab active/inactive locking for content process

No, this is wrong.  In general, we don't want to disable locking for the content process.  Fennec is not going to be the only thing using content processes.

Again, it sounds like you want locking to not force eager decode, or that you want locking to never happen at all (they're not the same thing; they behave differently for things that were once in the viewport but aren't anymore).  Which of those do you actually want?
Attachment #504682 - Flags: review?(bzbarsky) → review-
I want both... no locking, because locked image stays in memory and wasting memory (less priority)
And I want decode only on paint request, possibly discard timer could be bigger... and of course no decoding on load.
OK, then I think we should just have a preference for image locking, which fennec can set to false instead of assuming things about content processes.
Comment on attachment 504685 [details] [diff] [review]
Update CSS viewport

this is already landed to m-b
Attachment #504685 - Flags: review?(21)
> OK, then I think we should just have a preference for image locking, which
Preference is good idea
> fennec can set to false instead of assuming things about content processes.
I'm afraid we still should do it only in content process... because we want to keep Fennec UI images locked... (don't know how many we have there...)
If we want to ignore image locking in content processes when some pref is set, that makes sense.

What we should not do is assume that content processes means we're fennec and make fennec-only choices based on being in a content process.  That's where the pref comes in.
Attached patch Tricky fix (obsolete) — Splinter Review
this should do the right things, but patch looks a bit tricky and ugly... should I create some new nsDocument function which return true or false and use it in layout?
Attachment #504682 - Attachment is obsolete: true
Attachment #505922 - Flags: feedback?
Attachment #505922 - Attachment is obsolete: true
Attachment #506463 - Flags: review?(bzbarsky)
Attachment #505922 - Flags: feedback?
Comment on attachment 506463 [details] [diff] [review]
No lock image by default in content process if pref == false

You don't need the nsDocument::Init change; that member is already false at that point, no?

The change to SetIsActive is insufficient, unless you really want pages to start locking images when coming out of bfcache.

I think the right place to put this check would be in nsDocument::SetImageLockingState and that we should call the pref "content.image.allow_locking".

And please put the "&&" before the linebreak?
Attachment #506463 - Flags: review?(bzbarsky) → review-
Assignee: nobody → romaxa
Attachment #504685 - Attachment is obsolete: true
Attachment #506463 - Attachment is obsolete: true
Attachment #508093 - Flags: review?(bzbarsky)
In what kind of scenario do we expect this to save us memory in Fennec? I did some simple tests with this patch (load a page with lots of images, browse to other pages, etc.) and I don't seem to see a significant change with this pref on or off.
this patch is not enough, we need also 
+pref("image.mem.decodeondraw", true);
+pref("image.mem.max_bytes_for_sync_decode", 15000000);

so you get bug 622475 URL loaded without OOM
Trying with those prefs+the patch, I ran planet.mozilla.org 3 times with and without allow_locking. There was basically no difference, except for 1 time that plugin-container was 30% smaller. Perhaps the last run was a fluke, or perhaps the correct behavior happened there? In any case, I can't see a clear and consistent difference with the patch+prefs.

What is expected to happen here - images will not be decoded unless actually shown? And with allow_locking=false, even when we do decode, we don't force it to stay in memory if not needed?
main goal of these patches is to not decode image which  are not visible on screen (or at least on off-scrren layer) on page load.
so if you load bug 622475 URL with current fennec we do decode into temp surface all images on load and keep them in memory while current tab is active.

with patch + pref we should decode only images which are visible on screen (offscreen), and keep them decoded for default time 15sec... and then drop...

if user scroll page down, and want to see other images on page then it will be decoded, and painted
make simple page with huge panorama image... move it to the bottom of long page, and check memory consumption on page loading, and after scroll down (reach image, force decode)
with default fennec (no patch) all images in content process are decoded on page load and stay in memory until tab switched, hence with dependent bug URL we are consuming ~300Mb (commited) while loading page
tracking-fennec: --- → 2.0+
Comment on attachment 508093 [details] [diff] [review]
No image locking for content process by pref

r=me.  Sorry for the lag...
Attachment #508093 - Flags: review?(bzbarsky) → review+
leaving this open.  not sure we want to take the decode-on-draw pref switch right now.  lets check this in post-beta-5, unless others object.
Given that I wrote all this stuff, I probably should have been following this more closely. I've been pretty swamped. :(

Anyway, I saw Crowder's blog post ( http://blog.mozilla.com/bcrowder/2011/02/15/decode-on-draw-for-fennec/ ) and I'm a bit confused here.

image.mem.decodeondraw isn't a hugely important pref. It only controls the _first_ decode of the image (whether it happens when the image is loaded from the network or at first paint). The steady-state behavior is governed by image discarding.

Is image.mem.discardable already set to true on Fennec?
-If so: Discarding is also governed image.mem.max_bytes_for_sync_decode. Given that the patch sets it to the value used on Desktop Firefox, what is it set to in the status quo?
-If not: image.mem.discardable is a much more effective pref for reducing memory footprint, since otherwise any image that was ever decoded stays decoded.


Crowder implies in his blog post that this bug adds the the prefs image.mem.decodeondraw and image.mem.max_bytes_for_sync_decode.  However, that's not the case. This patch allows support for the content.image.allow_locking pref, which governs whether we can discard (and decode on draw) images in the active tab. Was he mistaken or am I missing something?
> image.mem.decodeondraw isn't a hugely important pref. It only controls the
> _first_ decode of the image (whether it happens when the image is loaded from

first load behavior is most important goal of this bug, and without that pref I had images decoded on load into temporary surface... need double check that

> 
> Is image.mem.discardable already set to true on Fennec?
yes, it is not changed in fennec prefs, so it is coming from greprefs.js

> -If so: Discarding is also governed image.mem.max_bytes_for_sync_decode. Given
> that the patch sets it to the value used on Desktop Firefox, what is it set to

image.mem.max_bytes_for_sync_decode - is just workaround for problem which I haven't investigated deeply yet. offscreen images after scrolling decoded in async mode are not painted properly. force sync helping to workaround that problem

> -If not: image.mem.discardable is a much more effective pref for reducing
> memory footprint, since otherwise any image that was ever decoded stays
> decoded.

yes, but goal is to prevent first decoding for offscreen images

> content.image.allow_locking pref, which governs whether we can discard (and

yes, blog post should mention that this pref should be enabled too.
(In reply to comment #37)

> image.mem.max_bytes_for_sync_decode - is just workaround for problem which I
> haven't investigated deeply yet. offscreen images after scrolling decoded in
> async mode are not painted properly. force sync helping to workaround that
> problem

I guess I'm confused as to why that line in the patch does anything. image.mem.max_bytes_for_sync_decode is already set to 150000 in all.js. Why does this need to be set again in the mobile prefs?
(In reply to comment #38)
> image.mem.max_bytes_for_sync_decode is already set to 150000 in all.js.

The patch changes it from 150,000 (150 KB) to 15,000,000 (15 MB).
(In reply to comment #39)
> (In reply to comment #38)
> > image.mem.max_bytes_for_sync_decode is already set to 150000 in all.js.
> 
> The patch changes it from 150,000 (150 KB) to 15,000,000 (15 MB).

Oh, whoops. Missed the zeros. ;-)

Just FYI - synchronously decoding a 15 meg image will lock things up considerably. I understand that it's just a workaround though.
lock things up in the child process, correct?
> Just FYI - synchronously decoding a 15 meg image will lock things up
> considerably. I understand that it's just a workaround though.

> lock things up in the child process, correct?

yep, it is child process lock, this is workaround and should not be used for single process firefox.
Depends on: 634616
Bobby:  Feel free to comment on the blog if I'm getting stuff wrong; I surely am, as I haven't been reviewing the patches very carefully.  Thanks!
hmm, tested with latest m-c, m-b tip and my test page works fine even without sync decoding...
using only prefs 
pref("image.mem.decodeondraw", true);
pref("content.image.allow_locking", false);

tested on planet.gnome.org, all images loaded in async mode without any problems

also noticed that we don't free memory anymore (with and without prefs) allocated by images...

will check what is going on with discarding timer...
ok, default discrad timeout was recently increased from 10 seconds -> 120 seconds :(.
Attachment #504683 - Attachment is obsolete: true
Attachment #513283 - Flags: review?(mark.finkle)
Attachment #513283 - Flags: review?(mark.finkle) → review+
I think that wraps up this bug
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 647802
Depends on: 691169
You need to log in before you can comment on or make changes to this bug.