Closed Bug 634844 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Plug-ins, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: roc, Assigned: roc)

References

(Blocks 1 open bug)

Details

(Whiteboard: [hardblocker][has patch])

Attachments

(2 files, 1 obsolete file)

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!).
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.
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.
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.
Assignee: nobody → matt.woodrow+bugzilla
blocking2.0: ? → final+
Whiteboard: [hardblocker]
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.
We also probably want to port lazy uploading to d3d10, but it clearer to do that in a separate bug.
(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.
(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.
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?
Attachment #513595 - Flags: review?
Attachment #513595 - Flags: review? → review?(benjamin)
Won't this patch make IsUpToDate always return false on Mac when we're using IOSurfaces?
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.
Bug 631388 has a patch to add GetImageSize.
Assignee: matt.woodrow+bugzilla → roc
Attachment #513595 - Attachment is obsolete: true
Attachment #513730 - Flags: review?(jones.chris.g)
Attachment #513595 - Flags: review?(benjamin)
Whiteboard: [hardblocker] → [hardblocker][has patch]
Passed reftests on try.
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][needs review]
Whiteboard: [hardblocker][has patch][needs review] → [hardblocker][has patch][needs review cjones]
Did you get tp4 numbers from try?
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.
Attachment #513730 - Flags: review?(jones.chris.g) → review+
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.
Attachment #513731 - Flags: review?(jones.chris.g) → review+
(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.)
(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.
(Oops, actually I was CC'd, I just missed your comment.)
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.
Whiteboard: [hardblocker][has patch][needs review cjones] → [hardblocker][has patch][needs landing]
http://hg.mozilla.org/mozilla-central/rev/3ce676745ec1
http://hg.mozilla.org/mozilla-central/rev/8e377921a023
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker][has patch][needs landing] → [hardblocker][has patch]
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
You need to log in before you can comment on or make changes to this bug.