935380 - Make aOffset a property of the render target rather than passing it around everywhere

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Matt Woodrow (:mattwoodrow)]

https://tbpl.mozilla.org/?tree=Try&rev=f3cbb2752923

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Remove INIT_MODE_COPY

Decided that this would be a useful simplification for the work that dvander is doing on BasicCompositor and hacked it up quickly.

This part removes INIT_MODE_COPY since it's not actually valid for callers to pass it (they need to use CreateRenderTargetFromSource).

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Pass the actual RenderTarget rect to CreateRenderTarget

Note to self: Comment this better asap.

This makes the aRect parameter to CreateRenderTarget represent the actual rect that the the render target represents, and adds an extra parameter for the copy version of where in the source buffer to copy from.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Make CompositingRenderTarget store it's origin

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Make the Compositors use the RenderTarget's origin instead of aOffset

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Remove aOffset from everywhere

Comment 7

[Nick Cameron [:nrc]]

Comment on attachment 827831 [details] [diff] [review]
Part 1: Remove INIT_MODE_COPY

Review of attachment 827831 [details] [diff] [review]:
-----------------------------------------------------------------

I would marginally prefer having a flag rather than a bool as a param for CreateFBOWithTexture, but r=me either way

::: gfx/layers/opengl/CompositorOGL.h
@@ +273,5 @@
>     * of the type returned by FBOTextureTarget; different
>     * shaders are required to sample from the different
>     * texture types.
>     */
> +  void CreateFBOWithTexture(const gfx::IntRect& aRect, bool aCopyFromSource,

Using a bool rather than a flag here is a little bit unpleasant.

Comment 8

[Nick Cameron [:nrc]]

Comment on attachment 827832 [details] [diff] [review]
Part 2: Pass the actual RenderTarget rect to CreateRenderTarget

Review of attachment 827832 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Compositor.h
@@ +250,5 @@
>     */
>    virtual TemporaryRef<CompositingRenderTarget>
>    CreateRenderTargetFromSource(const gfx::IntRect& aRect,
> +                               const CompositingRenderTarget* aSource,
> +                               const gfx::IntPoint& aSourcePoint) = 0;

Please add a comment for aSourcePoint. And name it consistently - it is aSourceOffset in some places

Comment 9

[Nick Cameron [:nrc]]

Comment on attachment 827833 [details] [diff] [review]
Part 3: Make CompositingRenderTarget store it's origin

Review of attachment 827833 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +41,5 @@
>  
>  CompositingRenderTargetD3D9::CompositingRenderTargetD3D9(IDirect3DTexture9* aTexture,
>                                                           SurfaceInitMode aInit,
> +                                                         const gfx::IntRect& aRect)
> +  : CompositingRenderTarget(aRect.TopLeft()) 

trailing whitespace

::: gfx/layers/opengl/CompositingRenderTargetOGL.h
@@ +84,5 @@
>                          const gfx::IntSize& aSize,
>                          const gfxMatrix& aTransform)
>    {
>      RefPtr<CompositingRenderTargetOGL> result
> +      = new CompositingRenderTargetOGL(aCompositor, gfx::IntPoint(0, 0), 0, 0);

This looks like it might benefit from having default params, but I don't feel too strongly about it

Comment 10

[Nick Cameron [:nrc]]

Comment on attachment 827835 [details] [diff] [review]
Part 5: Remove aOffset from everywhere

Review of attachment 827835 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! I like the change. (a whole bunch of the nits below are not due to this patch, but we might as well take this opportunity to tidy up the code a little bit).

::: gfx/layers/Compositor.h
@@ +281,5 @@
>     * Tell the compositor to actually draw a quad. What to do draw and how it is
>     * drawn is specified by aEffectChain. aRect is the quad to draw, in user space.
>     * aTransform transforms from user space to screen space. aOffset is the
>     * offset of the render target from 0,0 of the screen. If texture coords are
>     * required, these will be in the primary effect in the effect chain.

remove the comment about aOffset too

::: gfx/layers/basic/BasicCompositor.cpp
@@ +332,5 @@
>  
>  void
>  BasicCompositor::DrawQuad(const gfx::Rect& aRect, const gfx::Rect& aClipRect,
>                            const EffectChain &aEffectChain,
> +                          gfx::Float aOpacity, const gfx::Matrix4x4 &aTransform)

same thing with one arg per line

::: gfx/layers/basic/BasicCompositor.h
@@ +79,5 @@
>    }
>  
>    virtual void DrawQuad(const gfx::Rect& aRect, const gfx::Rect& aClipRect,
>                          const EffectChain &aEffectChain,
> +                        gfx::Float aOpacity, const gfx::Matrix4x4 &aTransform) MOZ_OVERRIDE;

seeing as we're changing the arg list, lets put one arg per line here, it will be easier to read

::: gfx/layers/composite/ImageHost.cpp
@@ +121,5 @@
>      it->EndTileIteration();
>      // layer border
>      GetCompositor()->DrawDiagnostics(DIAGNOSTIC_IMAGE,
>                                       gfxPictureRect, aClipRect,
> +                                     aTransform);    

trailing whitespace

::: gfx/layers/d3d9/CompositorD3D9.cpp
@@ +192,5 @@
>  
>  void
>  CompositorD3D9::DrawQuad(const gfx::Rect &aRect, const gfx::Rect &aClipRect,
>                           const EffectChain &aEffectChain,
> +                         gfx::Float aOpacity, const gfx::Matrix4x4 &aTransform)

one arg per line again

::: gfx/layers/d3d9/CompositorD3D9.h
@@ +56,5 @@
>    virtual void SetDestinationSurfaceSize(const gfx::IntSize& aSize) MOZ_OVERRIDE {}
>  
>    virtual void DrawQuad(const gfx::Rect &aRect, const gfx::Rect &aClipRect,
>                          const EffectChain &aEffectChain,
> +                        gfx::Float aOpacity, const gfx::Matrix4x4 &aTransform) MOZ_OVERRIDE;

and again

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +984,5 @@
>  
>  void
>  CompositorOGL::DrawQuad(const Rect& aRect, const Rect& aClipRect,
>                          const EffectChain &aEffectChain,
> +                        Float aOpacity, const gfx::Matrix4x4 &aTransform)

and again

::: gfx/layers/opengl/CompositorOGL.h
@@ +95,5 @@
>    virtual CompositingRenderTarget* GetCurrentRenderTarget() MOZ_OVERRIDE;
>  
>    virtual void DrawQuad(const gfx::Rect& aRect, const gfx::Rect& aClipRect,
>                          const EffectChain &aEffectChain,
> +                        gfx::Float aOpacity, const gfx::Matrix4x4 &aTransform) MOZ_OVERRIDE;

and again

Comment 11

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/021d1ed687ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/614ee414ef7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a46cdb5caef
https://hg.mozilla.org/integration/mozilla-inbound/rev/026dacdf8a25
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e339fbf953

Comment 12

[Ahmad Saleem]

Part 5 landed with incorrect bug number on m-i.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/021d1ed687ff
https://hg.mozilla.org/mozilla-central/rev/614ee414ef7e
https://hg.mozilla.org/mozilla-central/rev/5a46cdb5caef
https://hg.mozilla.org/mozilla-central/rev/026dacdf8a25