634844 - Non-animated plugins constantly reupload image with accelerated layers and async rendering

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I need to verify this with in a debugger, but looking at the code it seems to me that each call to nsObjectFrame::BuildLayer calls down to PluginInstanceParent::GetImage, which creates a new CAIRO_SURFACE image on Windows. This will result in the plugin surface being re-uploaded each time we paint, even if nothing has changed. This is pretty bad, for example it will probably hurt scrolling significantly when a plugin is visible (which is pretty often for Flash ads!).

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Is this related to a recent change? A regression from 3.6 or an earlier beta? I understand that bad perf is bad, but hard to tell why we're being asked to block on this.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There are softblocker bugs like bug 626245 that are quite possibly caused by this. It's probably easier to fix this and retest bug 626245 than to analyze 626245 in more detail.

Comment 3

[Benjamin Smedberg]

This looks like a recent regression from http://hg.mozilla.org/mozilla-central/rev/b0c3b563b2fa (bug 591687) where we switched from calling GetSurface every time (which is cached) to calling GetImage (which is not?)

I've noticed blip.tv being very choppy yesterday and today, so I'm going to tentatively blame this bug and mark this a hardblocker.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yeah, I saw in bug 635004 that we were creating and uploading 3 or 4 textures per frame rendered by the plugin before even painting, and then on painting creating and uploading another texture.  (There were other things going wrong in that bug that aren't common, probably.)  This might be fix enough for bug 635091.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We also probably want to port lazy uploading to d3d10, but it clearer to do that in a separate bug.

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to comment #5)
> We also probably want to port lazy uploading to d3d10, but it clearer to do
> that in a separate bug.

Theoretically the quick uploading in D3D10 is significantly better than delayed uploading. If we want to do something like this in D3D10 we should add a parameter to SetData in my opinion to trigger lazy/immediate uploading.

Comment 7

[Matt Woodrow (:mattwoodrow)]

(In reply to comment #3)
> This looks like a recent regression from
> http://hg.mozilla.org/mozilla-central/rev/b0c3b563b2fa (bug 591687) where we
> switched from calling GetSurface every time (which is cached) to calling
> GetImage (which is not?)
> 
> I've noticed blip.tv being very choppy yesterday and today, so I'm going to
> tentatively blame this bug and mark this a hardblocker.

That patch shouldn't have changed the behaviour of BuildLayer at all. The changes to IsUpToDate() are likely a regression though.

I think to fix this we can revert IsUpToDate() to the old code, the GetSurface() path still exists and should work fine.

For BuildLayer we can avoid calling SetCurrentImage() (or maybe avoid calling anything?) if the dirty rect is empty.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Revert IsUpToDate to use the cached surface

This should fix any regressions caused and make IsUpToDate() fast again.

For part 2, I'm not sure exactly under what conditions we can skip the upload. Is checking if the dirty rect passed to BuildLayer being empty enough?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Won't this patch make IsUpToDate always return false on Mac when we're using IOSurfaces?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

RecvNPN_InvalidateRect is called whenever the surface changes, and nsObjectFrame::InvalidateRect does a SetCurrentImage. So I think nsObjectFrame::BuildLayer should only do a SetCurrentImage if the image container has no current image (i.e. it's the first time we're rendering the plugin with this container). We can then get rid of the NS_OBJECT_NEEDS_SET_IMAGE bit.

For IsUpToDate() I think we should add alongside nsIPluginInstance::GetImage a GetImageSize method that just returns the gfxASurface or MacIOSurface size, without creating an image of course. I also need this for bug 631388 anyway.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 631388 has a patch to add GetImageSize.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1 alternative: Use GetImageSize

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Don't create new image in BuildLayer if the container already has an image

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Passed reftests on try.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Did you get tp4 numbers from try?

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 513730 [details] [diff] [review]
Part 1 alternative: Use GetImageSize

>+    nsIntSize size;
>+    if (NS_FAILED(inst->GetImageSize(&size)) ||
>+        size != nsIntSize(mPluginWindow->width, mPluginWindow->height))
>       return PR_FALSE;
>-    }

I prefer the style |return (NS_SUCCEEDED(...) && ...);| but don't care that much.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 513731 [details] [diff] [review]
Part 2: Don't create new image in BuildLayer if the container already has an image

I'd sort of hoped we could fix this bug so as to also not unnecessarily upload for animated plugins, but we don't know if that's a problem for d3d10 yet.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #15)
> Did you get tp4 numbers from try?

I did not. I don't anticipate any regression here, this should be pure win, so we'll see what m-c says.

(In reply to comment #17)
> Comment on attachment 513731 [details] [diff] [review]
> Part 2: Don't create new image in BuildLayer if the container already has an
> image
> 
> I'd sort of hoped we could fix this bug so as to also not unnecessarily upload
> for animated plugins, but we don't know if that's a problem for d3d10 yet.

I'm not sure what you mean. If the plugin surface changes, we have to re-upload it. (At some point we should only upload the changed part, but that requires significantly more work --- bug 626520.)

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Sorry, wasn't CC'd.)

(In reply to comment #18)
> I'm not sure what you mean. If the plugin surface changes, we have to re-upload
> it. (At some point we should only upload the changed part, but that requires
> significantly more work --- bug 626520.)

We always have the plugin surface in system memory atm.  Every time InvalidateRect() is called, we're forced to create a new Image, which except in d3d9 forces us to create a new texture and upload.  InvalidateRect() can be called many times before actually painting the window.  So we currently end up wasting work by creating potentially many unused textures, which bug 626520 would help fix, but we also waste work in potentially uploading the same pixels many times before repainting.  That's not necessary because we always have the most up-to-date copy of them in system memory.  I think we could "fix" both issues here by lazy creation of the Image in nsObjectFrame, i.e. when it's needed to repaint, but it's not clear that would be a perf win with d3d10.  That has tradeoffs vs. the approach of bug 626520 too.  These patches are definitely an improvement so no need to bite all that off here.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Oops, actually I was CC'd, I just missed your comment.)

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

D3D9 does not upload until the layer system actually renders the image. D3D10 does. I think this is something that can be adequately handled in the layer system using lazy uploading if we decide it's a performance win.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/3ce676745ec1
http://hg.mozilla.org/mozilla-central/rev/8e377921a023

Comment 23

[orangezilla]

This has been reported to have fixed the Nike sites on bug 634380, now marked as resolved

thanks to Keldorn on mozillazine
http://forums.mozillazine.org/viewtopic.php?f=23&t=2116361&p=10474673#p10474673