Add shadow-layer support to d3d9 backend

VERIFIED FIXED

Status

()

Core
Graphics
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: cjones, Assigned: BenWa)

Tracking

(Blocks: 1 bug)

unspecified
x86
Windows XP
Points:
---

Firefox Tracking Flags

(firefox7 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

This will be basic --> d3d9 forwarding.  Same idea as the GL backend, with d3d9 interfaces instead.
(Assignee)

Updated

6 years ago
Assignee: nobody → bgirard
(Assignee)

Comment 1

6 years ago
Created attachment 537847 [details] [diff] [review]
ShadowD3D9_wip.patch

Stubbed out D3D9 Shadow Layers and minor refactoring analogous to ShadowOGL refactoring.
(Assignee)

Comment 2

6 years ago
Created attachment 539876 [details] [diff] [review]
ShadowD3D9_wip2.patch

Patch in a working state. Fixed the position problem. Still missing ShadowCanvasLayer and a few clean up method to be implemented.
Attachment #537847 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
Created attachment 541525 [details] [diff] [review]
ShadowD3D9_v1.patch

All layers working (pages, video, canvas)
Attachment #539876 - Attachment is obsolete: true
(Assignee)

Comment 4

6 years ago
Created attachment 541528 [details] [diff] [review]
ShadowD3D9_v1.patch
Attachment #541525 - Attachment is obsolete: true
Attachment #541528 - Flags: review?(jones.chris.g)
(Assignee)

Comment 5

6 years ago
Created attachment 542296 [details]
Screenshot

Here's a screenshot. It also shows a slight rendering problem where words have a dot after them. Bas said he had an idea what it could be, we will look at it tomorrow.
Comment on attachment 541528 [details] [diff] [review]
ShadowD3D9_v1.patch

I've looked at your D3D9 API usage, but I'm in no position to give it
a thorough review.  Just a word of warning.

Also, for posterity's sake, me reviewing this patch is a bit
incestuous because it borrows heavily from the GL stuff.  But let's
keep the ball rolling.

>+void 
>+ShadowCanvasBufferD3D9::Upload(gfxASurface* aUpdate)
>+{
>+  D3DLOCKED_RECT r;
>+  mTexture->LockRect(0, &r, NULL, 0);
>+
>+  mTexture->UnlockRect(0);

It's almost always a good idea to invest in RAII helpers for these
kinds of patterns.  But that could come in a followup, if at all.

>+void
>+ShadowCanvasLayerD3D9::Initialize(const Data& aData)
>+{
>+  NS_RUNTIMEABORT("Incompatibe surface type");

"Incompatible".  But please say something more descriptive like,
"Non-shadow layer API unexpectedly used for shadow layer".

>+
>+void
>+ShadowCanvasLayerD3D9::Swap(const SurfaceDescriptor& aNewFront,
>+                           SurfaceDescriptor* aNewBack)
>+{
>+  if (aNewFront.type() == SharedImage::TSurfaceDescriptor) {

Assert this.

>+void 
>+ShadowImageBufferD3D9::Upload(gfxASurface* aUpdate)
>+{

This looks like pretty much the same code as ShadowCanvasBufferD3D9.
It's also pretty much the same thing as the TextureImage in GL-land.
Any chance we can share at least this with ShadowCanvasBufferD3D9?

>+void 
>+ShadowThebesBufferD3D9::Upload(gfxASurface* aUpdate, 
>+                         const nsIntRegion& aUpdated,
>+                         const nsIntRect& aRect, const nsIntPoint& aRotation)

This looks pretty familiar too ...

>+void
>+ShadowThebesLayerD3D9::SetFrontBuffer(const OptionalThebesBuffer& aNewFront,
>+                                     const nsIntRegion& aValidRegion,
>+                                     float aXResolution, float aYResolution)

With 637852 landed, we should strip resolution out of these
interfaces.  But we can do that in a followup (if Rob didn't already).

There's nothing in here I'm uncomfortable landing with.  Let's figure
out how to reduce code duplication though.
Attachment #541528 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 7

6 years ago
(In reply to comment #6)
> Comment on attachment 541528 [details] [diff] [review] [review]
> ShadowD3D9_v1.patch
> 
> I've looked at your D3D9 API usage, but I'm in no position to give it
> a thorough review.  Just a word of warning.
> 

Should I ask Bas to review it as well?

> There's nothing in here I'm uncomfortable landing with.  Let's figure
> out how to reduce code duplication though.

Right, I had tried something but I saw that it required an API change. I'll take another look to see if there's a better way.

If you'd like I can attempt to share at least an interface with the GL version.
(In reply to comment #7)
> (In reply to comment #6)
> > Comment on attachment 541528 [details] [diff] [review] [review] [review]
> > ShadowD3D9_v1.patch
> > 
> > I've looked at your D3D9 API usage, but I'm in no position to give it
> > a thorough review.  Just a word of warning.
> > 
> 
> Should I ask Bas to review it as well?

I think it's OK to land as-is.  There's nothing particularly complicated.

> 
> > There's nothing in here I'm uncomfortable landing with.  Let's figure
> > out how to reduce code duplication though.
> 
> Right, I had tried something but I saw that it required an API change. I'll
> take another look to see if there's a better way.
> 
> If you'd like I can attempt to share at least an interface with the GL
> version.

I would start by sharing code just between files in the d3d9 backend.  If it makes sense in the future, we could try to share code with the GL backend.  That might be something to discuss at the work week (longer-term plans, that is).
(Assignee)

Comment 9

6 years ago
Created attachment 542728 [details] [diff] [review]
Part 2: ShadowD3D9 Refactor

I put the changes in a different patch but I will qfold them when checking in.
(Assignee)

Updated

6 years ago
Attachment #542728 - Flags: review?(jones.chris.g)
Comment on attachment 542728 [details] [diff] [review]
Part 2: ShadowD3D9 Refactor

Mostly looks fine, even righteous, except

>+class LockTextureRectD3D9 
>+{
>+public:
>+  LockTextureRectD3D9(IDirect3DTexture9* aTexture) 
>+    : mTexture(aTexture)
>+  {
>+    mLockResult = mTexture->LockRect(0, &mR, NULL, 0);
>+  }

I think |mR| needs to be set somewhere, right?

Also, looks like you didn't hg add the ShadowBufferD3D9.* files.
(Assignee)

Comment 11

6 years ago
Created attachment 542812 [details] [diff] [review]
Part 2: ShadowD3D9 Refactor

(In reply to comment #10)
> I think |mR| needs to be set somewhere, right?
> 

I don't think so as it's a out parameter. I check the return value of the lock operation and only read |mR| if the call succeeded. 

> Also, looks like you didn't hg add the ShadowBufferD3D9.* files.

Fixed.
Attachment #542728 - Attachment is obsolete: true
Attachment #542728 - Flags: review?(jones.chris.g)
Attachment #542812 - Flags: review?(jones.chris.g)
(Assignee)

Comment 12

6 years ago
Created attachment 542817 [details] [diff] [review]
Part 2: ShadowD3D9 Refactor

Forgot qref.
Attachment #542812 - Attachment is obsolete: true
Attachment #542812 - Flags: review?(jones.chris.g)
Attachment #542817 - Flags: review?(jones.chris.g)
Attachment #542817 - Flags: review?(jones.chris.g) → review+
http://hg.mozilla.org/mozilla-central/rev/9efe252bb236
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
status-firefox7: --- → fixed

Comment 14

6 years ago
Build ID: Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Setting this bug as Verified. All the changes made are visible also in the Beta repository (i.e. the changes made to CanvasLayerD3D9.cpp) in accordance with the "status-firefox 7" flag.

Is there a possiblity to have also a test case attached that would reflect the changes/improvements or some steps/guidelines how to create one? Thanks
Status: RESOLVED → VERIFIED
No longer blocks: 641681
Blocks: 692975
You need to log in before you can comment on or make changes to this bug.