Asynchronous layer-based plugin painting on Windows

RESOLVED FIXED in mozilla2.0b8

Status

()

RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

unspecified
mozilla2.0b8
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(12 attachments, 4 obsolete attachments)

6.76 KB, patch
romaxa
: review+
Details | Diff | Splinter Review
21.15 KB, patch
jaas
: review+
Details | Diff | Splinter Review
5.15 KB, patch
karlt
: review-
Details | Diff | Splinter Review
26.92 KB, patch
jimm
: review+
Details | Diff | Splinter Review
1.25 KB, patch
karlt
: review+
Details | Diff | Splinter Review
15.09 KB, patch
jimm
: review+
karlt
: review+
Details | Diff | Splinter Review
9.06 KB, patch
jimm
: review+
Details | Diff | Splinter Review
1.38 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.85 KB, patch
jimm
: review+
Details | Diff | Splinter Review
4.68 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.10 KB, patch
jimm
: review+
Details | Diff | Splinter Review
1.04 KB, patch
jimm
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
Asynchronous layer-based plugin painting on Windows
(Assignee)

Updated

9 years ago
blocking2.0: --- → betaN+
(Assignee)

Comment 1

8 years ago
Created attachment 477151 [details] [diff] [review]
Part A, use bools, fix styling, rev. 1
Attachment #477151 - Flags: review?(jmathies)
(Assignee)

Comment 2

8 years ago
Created attachment 477281 [details] [diff] [review]
Make UseAsyncPainting simpler and stop delegating it all around, rev. 1
Attachment #477281 - Flags: review?(romaxa)
Comment on attachment 477151 [details] [diff] [review]
Part A, use bools, fix styling, rev. 1

I'm not a peer here, but looks fine to me.

There seems to be a missing agreement from peers on this type of change though. (I've been dinged for using false in dom/plugin code by other reviewers.) Maybe we need to standardize it and document it in out coding guidelines.

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
Attachment #477151 - Flags: review?(jmathies) → review+
(Assignee)

Comment 4

8 years ago
Comment on attachment 477151 [details] [diff] [review]
Part A, use bools, fix styling, rev. 1

This just aligns the usage with the rest of the code. Josh, can you mark official r+?
Attachment #477151 - Flags: review?(joshmoz)
Don't forget to respect the windowless "opaque" setting to turn off alpha recovery here.
We also need to test whether Flash sets the alpha values of the DIB correctly, in which case we don't need alpha recovery at all.

Updated

8 years ago
Blocks: 581725
Comment on attachment 477281 [details] [diff] [review]
Make UseAsyncPainting simpler and stop delegating it all around, rev. 1

works fine, and make sense while layers-non-ipc mode not implemented.
Attachment #477281 - Flags: review?(romaxa) → review+
Depends on: 599704

Comment 8

8 years ago
Comment on attachment 477151 [details] [diff] [review]
Part A, use bools, fix styling, rev. 1

>-    void UpdateWindowAttributes(PRBool aForceSetWindow = PR_FALSE);
>+    void UpdateWindowAttributes(bool aForceSetWindow = PR_FALSE);

You changed the argument type here but not the default value. Should be "false".
Attachment #477151 - Flags: review?(joshmoz) → review-
(Assignee)

Comment 9

8 years ago
Created attachment 479152 [details] [diff] [review]
Part A, use bools, fix styling, rev. 1.1

Was that all? No "r+ with this fixed"? :-(
Attachment #477151 - Attachment is obsolete: true
Attachment #479152 - Flags: review?(joshmoz)

Updated

8 years ago
Attachment #479152 - Flags: review?(joshmoz) → review+
Blocks: 596441
Blocks: 591409

Comment 10

8 years ago
Any progress?

Comment 11

8 years ago
Pardon my curiosity,
The symptoms in Bug 603040 Comment #3 (TLDR: Plugin gets drawn before page and FF GUI), are they related to this bug (will be fixed w. this)?
(Assignee)

Comment 12

8 years ago
Hera, no, that is unrelated.
(Assignee)

Comment 13

8 years ago
Created attachment 483207 [details] [diff] [review]
Part C - simpify the returning surface in show, rev. 1
Attachment #483207 - Flags: review?(joe)
(Assignee)

Comment 14

8 years ago
Comment on attachment 483207 [details] [diff] [review]
Part C - simpify the returning surface in show, rev. 1

karlt would be a better reviewer here. Please note that there is one behavior change: I'm no longer calling XSync when returning the old surface. I don't think it should be necessary, because the parent process never writes to that surface, only composites from it... or is it necessary because there might be a pending composite operation?
Attachment #483207 - Flags: review?(joe) → review?(karlt)
Comment on attachment 483207 [details] [diff] [review]
Part C - simpify the returning surface in show, rev. 1

>-    if (!SendShow(r, currSurf, &outSurf)) {
>+
>+    // Unused, except to possibly return a shmem to us
>+    SurfaceDescriptor returnSurf;
>+
>+    if (!SendShow(r, currSurf, &returnSurf)) {
>         return false;
>     }
> 
>     nsRefPtr<gfxASurface> tmp = mCurrentSurface;
>     mCurrentSurface = mBackSurface;
>     mBackSurface = tmp;
>     // Outdated back surface... not usable anymore due to changed plugin size.
>     // Dropping obsolete surface

I expect this part is fine, but IIUC outSurf/returnSurf is usually mBackSurface, so it is necessary to ensure that the browser has finished with returnSurf.

(In reply to comment #14)
> Please note that there is one behavior
> change: I'm no longer calling XSync when returning the old surface. I don't
> think it should be necessary, because the parent process never writes to that
> surface, only composites from it... or is it necessary because there might be a
> pending composite operation?

Yes.  The browser-side composite read is pending (in the X server) and XSync ensures that it happens.

The plugin must not delete and should not write to the surface before the X server has completed the composite read requested by the browser.
Attachment #483207 - Flags: review?(karlt) → review-
(Assignee)

Comment 16

8 years ago
Created attachment 484462 [details] [diff] [review]
Async Painting, checkpoint

This is a checkpoint that kinda-works. Our test plugin appears to paint correctly, as does the simple Flash at http://www.webwasp.co.uk/tutorials/a27-transparent/index.php

However, there are the following fairly serious problems:

1) vimeo and hulu refuse to repaint at all. I'm having trouble separating the main video on vimeo (which doesn't work) from the advertising, which works fine.

2) Silverlight doesn't work at all.

3) I haven't hooked up transparency at all yet. I'll be testing shortly to see whether DIBs with transparency work correctly.

I'll do some more work tomorrow, trying to create better testcases. I'd love assistance where possible, though!
(Assignee)

Updated

8 years ago
Depends on: 606285
(Assignee)

Comment 17

8 years ago
Created attachment 485146 [details] [diff] [review]
Part D - async painting on Windows (opaque), rev. 1
Attachment #484462 - Attachment is obsolete: true
Attachment #485146 - Flags: review?(jmathies)
(In reply to comment #17)
> Created attachment 485146 [details] [diff] [review]
> Part D - async painting on Windows (opaque), rev. 1

Can you post updated patches? All three fail to apply.
(Assignee)

Comment 19

8 years ago
I'm developing this against projects/maple, all except the latest patch are pushed there, and you should be able to apply the latest on top of it.
(Assignee)

Comment 20

8 years ago
Created attachment 485735 [details] [diff] [review]
Part C review comment 1

Since I'm committing as I go, here is the review comment addressed for part C.
Attachment #485735 - Flags: review?(karlt)
Comment on attachment 485146 [details] [diff] [review]
Part D - async painting on Windows (opaque), rev. 1

> namespace mozilla {
> namespace plugins {
> 
> struct SurfaceDescriptorX11 {
>   int XID;
>   int xrenderPictID;
>   gfxIntSize size;
> };
> 
>+struct SurfaceDescriptorWin {
>+  SharedMemoryHandle handle;
>+  gfxIntSize size;
>+};

While we're in here can we fix the warning on win builds for SurfaceDescriptorX11?

> 
>+static const PRUint32 kBytesPerPixel = 4;
>+
> PRUint32
>-SharedDIBWin::SetupBitmapHeader(PRUint32 aWidth, PRUint32 aHeight, PRUint32 aDepth, BITMAPINFOHEADER *aHeader)
>+SharedDIBWin::SetupBitmapHeader(PRUint32 aWidth, PRUint32 aHeight, BITMAPINFOHEADER *aHeader)


nit, move kBytesPerPixel to the top of the file.


I ran through all of my different windowless tests cases and didn't see any anomalies. Looks good!
Attachment #485146 - Flags: review?(jmathies) → review+
Comment on attachment 485735 [details] [diff] [review]
Part C review comment 1

/me notices this correctly moves the XSync to before the reply.
Attachment #485735 - Flags: review?(karlt) → review+
(Assignee)

Comment 23

8 years ago
Created attachment 486056 [details] [diff] [review]
Part E - Implement transparency, rev. 1
Attachment #486056 - Flags: review?(karlt)
Attachment #486056 - Flags: review?(jmathies)
(Assignee)

Comment 24

8 years ago
Created attachment 486063 [details] [diff] [review]
Part E - Implement transparency, rev. 1.1
Attachment #486056 - Attachment is obsolete: true
Attachment #486063 - Flags: review?(karlt)
Attachment #486063 - Flags: review?(jmathies)
Attachment #486056 - Flags: review?(karlt)
Attachment #486056 - Flags: review?(jmathies)
(Assignee)

Updated

8 years ago
Attachment #486063 - Flags: review?(karlt)
Attachment #486063 - Flags: review?(jmathies)
(Assignee)

Comment 25

8 years ago
Created attachment 486114 [details] [diff] [review]
Part E - Implement transparency, rev. 1.2
Attachment #486114 - Flags: review?(karlt)
Attachment #486114 - Flags: review?(jmathies)
So is part E assuming that all plugins will put alpha values in the DIB when they draw?
(Assignee)

Comment 27

8 years ago
Created attachment 486125 [details] [diff] [review]
Part F - remove silverlight positioning quirk, and make setwindow use the real window coordinates with a translation for painting, rev. 1
Attachment #486125 - Flags: review?(jmathies)
(Assignee)

Updated

8 years ago
Attachment #486063 - Attachment is obsolete: true
(Assignee)

Comment 28

8 years ago
Yes, I am assuming/requiring that the painting sets the alpha values. In practice this works with Flash. I'm working on a silverlight testcase, and I don't know of any other plugins that do transparent windowless.
(Assignee)

Comment 29

8 years ago
I'm having a problem with silverlight, the testcase is online here: http://office.smedbergs.us/sltest/silverlight.html

In this test, we are call AsyncSetWindow with bad coordinates because of the following sequence of events:

We call nsObjectFrame::FixupWindow very early on. At this point, window->type is the default windowed mode, because we haven't initialized it yet. So GetWindowOriginInPixels doesn't do the necessary coordinate transformation at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#1184

>	xul.dll!nsObjectFrame::FixupWindow(aSize={...})  Line 1043	C++
 	xul.dll!nsObjectFrame::Instantiate(aChannel=0x09039d68, aStreamListener=0x08fbbeac)  Line 2262	C++
 	xul.dll!nsObjectLoadingContent::OnStartRequest(aRequest=0x09039d68, aContext=0x00000000)  Line 706	C++
 	xul.dll!nsBaseChannel::OnStartRequest(request=0x098006d0, ctxt=0x00000000)  Line 712	C++
 	xul.dll!nsInputStreamPump::OnStateStart()  Line 441	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(stream=0x09172b20)  Line 397	C++
 	xul.dll!nsInputStreamReadyEvent::Run()  Line 113	C++
 	xul.dll!nsThread::ProcessNextEvent(mayWait=0x00000000, result=0x0030d3c4)  Line 547	C++
 	xul.dll!NS_ProcessNextEvent_P(thread=0x007959e8, mayWait=0x00000000)  Line 250	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(aDelegate=0x00799728)  Line 110	C++
 	xul.dll!MessageLoop::RunInternal()  Line 220	C++
 	xul.dll!MessageLoop::RunHandler()  Line 203	C++
 	xul.dll!MessageLoop::Run()  Line 177	C++
 	xul.dll!nsBaseAppShell::Run()  Line 186	C++
 	xul.dll!nsAppShell::Run()  Line 243	C++
 	xul.dll!nsAppStartup::Run()  Line 191	C++
 	xul.dll!XRE_main(argc=0x00000005, argv=0x00612280, aAppData=0x0078f5c0)  Line 3670	C++

Then almost immediately after, we set mPluginWindow->type to windowless at this stack:

>	xul.dll!nsPluginInstanceOwner::CreateWidget()  Line 6166	C++
 	xul.dll!nsPluginHost::InstantiateEmbeddedPlugin(aMimeType=0x0d3666f8, aURL=0x093a7798, aOwner=0x09711898)  Line 1071	C++
 	xul.dll!nsPluginStreamListenerPeer::OnStartRequest(request=0x09039d68, aContext=0x00000000)  Line 588	C++
 	xul.dll!nsObjectLoadingContent::OnStartRequest(aRequest=0x09039d68, aContext=0x00000000)  Line 738	C++

We then send AsyncSetWindow with the bad coordinates here:
>	xul.dll!mozilla::plugins::PluginInstanceParent::AsyncSetWindow(aWindow=0x094c4bf4)  Line 532	C++
 	xul.dll!mozilla::plugins::PluginModuleParent::AsyncSetWindow(instance=0x092a397c, window=0x094c4bf4)  Line 624	C++
 	xul.dll!nsNPAPIPluginInstance::AsyncSetWindow(window=0x094c4bf4)  Line 851	C++
 	xul.dll!nsPluginStreamListenerPeer::OnStartRequest(request=0x09039d68, aContext=0x00000000)  Line 610	C++
 	xul.dll!nsObjectLoadingContent::OnStartRequest(aRequest=0x09039d68, aContext=0x00000000)  Line 738	C++

Should we perhaps be forcing a call to nsObjectFrame::FixupWindow again after nsPluginInstanceOwner::CreateWidget? I suspect that this bug is only visible on <object> silverlight, not <embed>, because of the ordering of how streams are handled.
(Assignee)

Comment 31

8 years ago
Created attachment 486176 [details] [diff] [review]
Part G - Call FixupWindow again, rev. 1
Attachment #486176 - Flags: review?(roc)
Comment on attachment 486125 [details] [diff] [review]
Part F - remove silverlight positioning quirk, and make setwindow use the real window coordinates with a translation for painting, rev. 1

Don't forget to remove QUIRK_SILVERLIGHT_WINLESS_INPUT_TRANSLATION from the header.
Attachment #486125 - Flags: review?(jmathies) → review+
Wasn't that silverlight quirk added for OOPP bug 547353?
(Assignee)

Comment 34

8 years ago
Created attachment 486183 [details] [diff] [review]
Part H - make silverlight transparent by default, rev. 1

I had to keep the quirk for transparency, but I renamed it.
Attachment #486183 - Flags: review?(jmathies)
Comment on attachment 486183 [details] [diff] [review]
Part H - make silverlight transparent by default, rev. 1

+        // Silverlight assumes it is transparent in windowless moe.

nit - 'moe'
Attachment #486183 - Flags: review?(jmathies) → review+
So to test the whole series on maple, parts E -> H?
(Assignee)

Comment 37

8 years ago
Created attachment 486332 [details] [diff] [review]
Readback from Windows surfaces, rev. 1

Oops, I forgot to attach this earlier, it implements readback for Windows surfaces so that we don't repaint the entire area each time. It goes between E and F.
Attachment #486332 - Flags: review?(jmathies)
Some issues to be worked out, either here or in another bug:

1) the flash context menu is positioned wrong on hulu, and the main banner isn't painting.
2) we lost mouse cursor changes and the context menu on silverlight.net
3) I still see drawing anomalies on that surface site. They tend to show up down at the bottom edge as drawing artifacts. 

We went through all these bugs when we first implemented ipc drawing. I'm not sure if we want to refile and fix all those issues or try to track as many as we can down here before this lands.
(Assignee)

Comment 39

8 years ago
Hulu homepage is bug 606285.

Context menu stuff I'll deal with here, since we aren't calling the standard SetWindow path any more we're skipping our hooks.

I'm not sure about the drawing anomalies, I fixed the only ones I saw with part H.
(Assignee)

Comment 40

8 years ago
Created attachment 486395 [details] [diff] [review]
Part J - Fix testplugin alpha painting, rev. 1
Attachment #486395 - Flags: review?(jmathies)
(Assignee)

Comment 41

8 years ago
Created attachment 486399 [details] [diff] [review]
Part K - set up quirks, rev. 1
Attachment #486399 - Flags: review?(jmathies)

Updated

8 years ago
Attachment #486399 - Flags: review?(jmathies) → review+
Comment on attachment 486332 [details] [diff] [review]
Readback from Windows surfaces, rev. 1

Normally I'd say someone like roc, joe, or jeff should look this over, but since it's a copy paste I suppose I can approve it.
Attachment #486332 - Flags: review?(jmathies) → review+

Updated

8 years ago
Attachment #486114 - Flags: review?(jmathies) → review+
Comment on attachment 486395 [details] [diff] [review]
Part J - Fix testplugin alpha painting, rev. 1

per irc conversation, need to confirm everything still succeeds for non-async, and non-ipc enabled test runs.
Attachment #486395 - Flags: review?(jmathies) → review+
Comment on attachment 486114 [details] [diff] [review]
Part E - Implement transparency, rev. 1.2

I'm assuming you just wanted my review on the mochitest so r+ on that.
I'm not really familiar with the other code, so haven't studied that.
I'd tend to prefer passing an ImageFormat or other enum instead the bool parameter, but I assume you didn't ask my review on that.
Attachment #486114 - Flags: review?(karlt) → review+
(Assignee)

Updated

8 years ago
Depends on: 607958
(Assignee)

Updated

8 years ago
Depends on: 607972
If we created a way to pass D3D surfaces via IPC, then we could improve this further by having the plugin-container responsible for uploading the plugin content to D3D when acceleration is enabled --- which would be a big step towards implementing our proposal for D3D plugin rendering with NPAPI. Should we have a followup bug for that?
(Assignee)

Comment 47

8 years ago
Yes. I'm unlikely to implement it, though. I'm also going to file a followup on refactoring this so that in-process plugins use the asynchronous rendering path also.
Depends on: 611206
Target Milestone: --- → mozilla2.0b8

Updated

8 years ago
Depends on: 611698
Depends on: 611970

Updated

8 years ago
Depends on: 612686

Updated

8 years ago
Depends on: 612515

Updated

8 years ago
Depends on: 613406

Updated

8 years ago
Depends on: 613915

Comment 48

8 years ago
Could this have caused bug 617480?

Updated

8 years ago
Depends on: 611539

Updated

8 years ago
Depends on: 621495

Updated

8 years ago
Depends on: 625284

Updated

8 years ago
Depends on: 626177

Updated

8 years ago
Depends on: 626336

Updated

8 years ago
Depends on: 629012

Updated

8 years ago
Depends on: 627469
Depends on: 636244
You need to log in before you can comment on or make changes to this bug.