Closed Bug 935380 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Details

(Whiteboard: [qa-])

Attachments

(5 files)

No description provided.
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).
Attachment #827831 - Flags: review?(ncameron)
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.
Attachment #827832 - Flags: review?(ncameron)
Attachment #827835 - Flags: review?(ncameron)
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.
Attachment #827831 - Flags: review?(ncameron) → review+
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
Attachment #827832 - Flags: review?(ncameron) → review+
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
Attachment #827833 - Flags: review?(ncameron) → review+
Attachment #827834 - Flags: review?(ncameron) → review+
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
Attachment #827835 - Flags: review?(ncameron) → review+
Assignee: nobody → matt.woodrow
Part 5 landed with incorrect bug number on m-i.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.