Last Comment Bug 648480 - Add shadow-layer support to d3d9 backend
: Add shadow-layer support to d3d9 backend
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Benoit Girard (:BenWa)
:
:
Mentors:
Depends on:
Blocks: 692975
  Show dependency treegraph
 
Reported: 2011-04-07 21:39 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2011-10-07 15:42 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
ShadowD3D9_wip.patch (26.61 KB, patch)
2011-06-07 12:46 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
ShadowD3D9_wip2.patch (40.84 KB, patch)
2011-06-16 13:04 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
ShadowD3D9_v1.patch (53.18 KB, patch)
2011-06-23 15:24 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
ShadowD3D9_v1.patch (52.86 KB, patch)
2011-06-23 15:34 PDT, Benoit Girard (:BenWa)
cjones.bugs: review+
Details | Diff | Splinter Review
Screenshot (442.39 KB, image/png)
2011-06-27 15:39 PDT, Benoit Girard (:BenWa)
no flags Details
Part 2: ShadowD3D9 Refactor (26.00 KB, patch)
2011-06-28 23:00 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 2: ShadowD3D9 Refactor (26.00 KB, patch)
2011-06-29 07:21 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 2: ShadowD3D9 Refactor (33.36 KB, patch)
2011-06-29 07:38 PDT, Benoit Girard (:BenWa)
cjones.bugs: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-07 21:39:10 PDT
This will be basic --> d3d9 forwarding.  Same idea as the GL backend, with d3d9 interfaces instead.
Comment 1 Benoit Girard (:BenWa) 2011-06-07 12:46:22 PDT
Created attachment 537847 [details] [diff] [review]
ShadowD3D9_wip.patch

Stubbed out D3D9 Shadow Layers and minor refactoring analogous to ShadowOGL refactoring.
Comment 2 Benoit Girard (:BenWa) 2011-06-16 13:04:44 PDT
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.
Comment 3 Benoit Girard (:BenWa) 2011-06-23 15:24:15 PDT
Created attachment 541525 [details] [diff] [review]
ShadowD3D9_v1.patch

All layers working (pages, video, canvas)
Comment 4 Benoit Girard (:BenWa) 2011-06-23 15:34:29 PDT
Created attachment 541528 [details] [diff] [review]
ShadowD3D9_v1.patch
Comment 5 Benoit Girard (:BenWa) 2011-06-27 15:39:58 PDT
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 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-27 17:53:06 PDT
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.
Comment 7 Benoit Girard (:BenWa) 2011-06-27 18:01:16 PDT
(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.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-27 18:05:21 PDT
(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).
Comment 9 Benoit Girard (:BenWa) 2011-06-28 23:00:13 PDT
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.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-29 01:34:27 PDT
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.
Comment 11 Benoit Girard (:BenWa) 2011-06-29 07:21:33 PDT
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.
Comment 12 Benoit Girard (:BenWa) 2011-06-29 07:38:18 PDT
Created attachment 542817 [details] [diff] [review]
Part 2: ShadowD3D9 Refactor

Forgot qref.
Comment 13 Marco Bonardo [::mak] 2011-07-04 11:43:30 PDT
http://hg.mozilla.org/mozilla-central/rev/9efe252bb236
Comment 14 AndreiD[QA] 2011-08-25 04:39:25 PDT
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

Note You need to log in before you can comment on or make changes to this bug.