Last Comment Bug 702878 - [Azure] Extend gfx::2d API for Azure Thebes wrapper
: [Azure] Extend gfx::2d API for Azure Thebes wrapper
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: Bas Schouten (:bas.schouten)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-15 21:30 PST by Bas Schouten (:bas.schouten)
Modified: 2011-12-28 11:12 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Extend gfx::2d API (47.46 KB, patch)
2011-11-16 09:38 PST, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Extend gfx::2d API v2 (51.72 KB, patch)
2011-12-22 18:24 PST, Bas Schouten (:bas.schouten)
bas: review+
roc: superreview+
Details | Diff | Splinter Review

Description Bas Schouten (:bas.schouten) 2011-11-15 21:30:54 PST

    
Comment 1 Bas Schouten (:bas.schouten) 2011-11-16 09:38:45 PST
Created attachment 574917 [details] [diff] [review]
Extend gfx::2d API

This patch has the additions to the API and the implementation for the Direct2D backend.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-16 14:19:16 PST
Comment on attachment 574917 [details] [diff] [review]
Extend gfx::2d API

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

::: gfx/2d/2D.h
@@ +154,3 @@
>   */
>  struct DrawSurfaceOptions {
> +  DrawSurfaceOptions(Filter aFilter = FILTER_LINEAR, bool aSurfaceClipped = false)

Make this an enum value, not a bool. Say

enum SamplingBounds {
  SAMPLING_UNBOUNDED,
  SAMPLING_BOUNDED
};

What exactly is the source rectangle in this case?

@@ +224,5 @@
>     */
>    LinearGradientPattern(const Point &aBegin,
>                          const Point &aEnd,
> +                        GradientStops *aStops,
> +                        const Matrix &aMatrix = Matrix())

Specify what this matrix is for?

@@ +292,4 @@
>      : mSurface(aSourceSurface)
>      , mExtendMode(aExtendMode)
> +    , mFilter(aFilter)
> +    , mMatrix(aMatrix)

Document the matrix

@@ +336,5 @@
>     */
>    virtual int32_t Stride() = 0;
>  
> +  /*
> +   * This fucntion is called after modifying the data on the source surface

typo

@@ +339,5 @@
> +  /*
> +   * This fucntion is called after modifying the data on the source surface
> +   * directly through the data pointer.
> +   */
> +  virtual void MarkDirty() {}

Er ... should users be able to modify the data of a SourceSurface? These are supposed to be immutable!

@@ +626,5 @@
>  
>    /*
> +   * This essentially takes a source pattern and a mask, and composites the
> +   * source pattern onto the destination surface using the alpha channel of
> +   * the mask pattern as a mask for the operation.

"essentially" is not a reassuring word here :-).

For SVG, we'll want a version of this that uses the luminance of the mask instead. But we can that later by adding a MaskOptions object.

@@ +709,5 @@
>     */
> +  virtual TemporaryRef<GradientStops>
> +    CreateGradientStops(GradientStop *aStops,
> +                        uint32_t aNumStops,
> +                        ExtendMode aExtendMode = EXTEND_CLAMP) const = 0;

Do we need EXTEND_MIRROR? I don't think we do, and I think we should remove it.

I think EXTEND_WRAP should be renamed to EXTEND_REPEAT.
Comment 3 Bas Schouten (:bas.schouten) 2011-11-23 10:25:19 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Comment on attachment 574917 [details] [diff] [review] [diff] [details] [review]
> Extend gfx::2d API
> 
> Review of attachment 574917 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/2D.h
> @@ +154,3 @@
> >   */
> >  struct DrawSurfaceOptions {
> > +  DrawSurfaceOptions(Filter aFilter = FILTER_LINEAR, bool aSurfaceClipped = false)
> 
> Make this an enum value, not a bool. Say
> 
> enum SamplingBounds {
>   SAMPLING_UNBOUNDED,
>   SAMPLING_BOUNDED
> };

Ok

> 
> What exactly is the source rectangle in this case?

The source rectangle as passed to DrawSurface? :)

> @@ +339,5 @@
> > +  /*
> > +   * This fucntion is called after modifying the data on the source surface
> > +   * directly through the data pointer.
> > +   */
> > +  virtual void MarkDirty() {}
> 
> Er ... should users be able to modify the data of a SourceSurface? These are
> supposed to be immutable!

I don't think so for DataSourceSurfaces? Not in my mind at least. There's a good usecase for users that want to have an in-memory surface and then do partial updates. Particularly in the case of small update areas it seems wasteful to have to create a new SourceSurface each time.

> @@ +626,5 @@
> >  
> >    /*
> > +   * This essentially takes a source pattern and a mask, and composites the
> > +   * source pattern onto the destination surface using the alpha channel of
> > +   * the mask pattern as a mask for the operation.
> 
> "essentially" is not a reassuring word here :-).

I can remove it? :-)

> 
> For SVG, we'll want a version of this that uses the luminance of the mask
> instead. But we can that later by adding a MaskOptions object.

Yeah, it's a little more complicated since we don't have native support for this, for example in D2D.

> 
> @@ +709,5 @@
> >     */
> > +  virtual TemporaryRef<GradientStops>
> > +    CreateGradientStops(GradientStop *aStops,
> > +                        uint32_t aNumStops,
> > +                        ExtendMode aExtendMode = EXTEND_CLAMP) const = 0;
> 
> Do we need EXTEND_MIRROR? I don't think we do, and I think we should remove
> it.

I'm not sure? Hardware supports it so I figured I'd leave it. Especially since Cairo also has it.

> 
> I think EXTEND_WRAP should be renamed to EXTEND_REPEAT.

Ok, I prefer wrap but don't feel strongly about it.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-23 14:05:51 PST
(In reply to Bas Schouten (:bas) from comment #3)
> I don't think so for DataSourceSurfaces? Not in my mind at least. There's a
> good usecase for users that want to have an in-memory surface and then do
> partial updates. Particularly in the case of small update areas it seems
> wasteful to have to create a new SourceSurface each time.

Yes. However, we want to share SourceSurfaces across threads and that's much easier to do if they're guaranteed immutable. It's also difficult for people who want to hang onto references to cached SourceSurfaces if some are immutable and others aren't.

Maybe we should have an ImmutableSourceSurface subclass that immutable source surfaces can belong to, an ImmutableDataSourceSurface under that, and an AsImmutable member? That might require too much class/code duplication. How about instead, a boolean IsImmutable() method, and a ToImmutable() method that returns a reference to an immutable snapshot of the surface (which could be the same surface if it's already immutable)? Then we say that only immutable SourceSurfaces can be shared across threads.

> > @@ +626,5 @@
> > >  
> > >    /*
> > > +   * This essentially takes a source pattern and a mask, and composites the
> > > +   * source pattern onto the destination surface using the alpha channel of
> > > +   * the mask pattern as a mask for the operation.
> > 
> > "essentially" is not a reassuring word here :-).
> 
> I can remove it? :-)

OK :-)

> > @@ +709,5 @@
> > >     */
> > > +  virtual TemporaryRef<GradientStops>
> > > +    CreateGradientStops(GradientStop *aStops,
> > > +                        uint32_t aNumStops,
> > > +                        ExtendMode aExtendMode = EXTEND_CLAMP) const = 0;
> > 
> > Do we need EXTEND_MIRROR? I don't think we do, and I think we should remove
> > it.
> 
> I'm not sure? Hardware supports it so I figured I'd leave it. Especially
> since Cairo also has it.

Actually SVG needs it. But let's call it EXTEND_REFLECT since that's what SVG and cairo call it.

> > I think EXTEND_WRAP should be renamed to EXTEND_REPEAT.
> 
> Ok, I prefer wrap but don't feel strongly about it.

SVG and cairo call it REPEAT.
Comment 5 Jeff Muizelaar [:jrmuizel] 2011-12-16 20:04:10 PST
Comment on attachment 574917 [details] [diff] [review]
Extend gfx::2d API

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

Sorry this took so long.

::: gfx/2d/2D.h
@@ +649,5 @@
> +   *
> +   * aRect The rect to clip to
> +   */
> +  virtual void PushClipRect(const Rect &aRect) = 0;
> +

It might be worth documenting the fact that this is still in user space and not device space.

::: gfx/2d/DrawTargetD2D.cpp
@@ +912,5 @@
> +
> +  RefPtr<ID2D1Layer> layer;
> +  rt->CreateLayer(byRef(layer));
> +  rt->PushLayer(D2D1::LayerParameters(D2D1::InfiniteRect(), NULL, D2D1_ANTIALIAS_MODE_PER_PRIMITIVE, D2D1::IdentityMatrix(), 1.0f, maskBrush), layer);
> +

This line could probably be split into multiple ones.

@@ +985,5 @@
> +
> +  PushedClip clip;
> +  // Do not store the transform, just store the device space rectangle directly.
> +  clip.mBounds = D2DRect(mTransform.TransformBounds(aRect));
> +

It seems like this should be using a plain Transform instead of a TransformBounds()

@@ +1785,5 @@
> +            // XXX - FIX ME!!
> +            MOZ_ASSERT(false);
> +            gfxDebug() << "Source surface used for pattern too large!";
> +            return NULL;
> +          }

I think this hunk would be good to put in a function if it's not too hard.

@@ +2051,5 @@
>    }
>  
>    D2D1_FACTORY_OPTIONS options;
>  #ifdef _DEBUG
> +  options.debugLevel = D2D1_DEBUG_LEVEL_WARNING;

Intentional?

::: gfx/2d/Matrix.h
@@ +158,5 @@
> +  }
> +private:
> +  static bool FuzzyEqual(Float aV1, Float aV2) {
> +    // XXX - Check if fabs does the smart thing and just negates the sign bit.
> +    return fabs(aV2 - aV1) < 1e-6;

fabs turns into
  andpd	0x00000004(%rip),%xmm0
on OS X at least

::: gfx/2d/SourceSurfaceD2DTarget.cpp
@@ +160,5 @@
>      D2D1::BitmapProperties(D2D1::PixelFormat(DXGIFormat(mFormat), AlphaMode(mFormat)));
>    hr = aRT->CreateSharedBitmap(IID_IDXGISurface, surf, &props, byRef(mBitmap));
>  
>    if (FAILED(hr)) {
> +    // This seems to happen for FORMAT_A8 sometimes...

Did we run into this with the cairo backend too? We should try to use ReportError to find out more information about when it fails.
Comment 6 Bas Schouten (:bas.schouten) 2011-12-17 06:50:24 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> Comment on attachment 574917 [details] [diff] [review]
> @@ +985,5 @@
> > +
> > +  PushedClip clip;
> > +  // Do not store the transform, just store the device space rectangle directly.
> > +  clip.mBounds = D2DRect(mTransform.TransformBounds(aRect));
> > +
> 
> It seems like this should be using a plain Transform instead of a
> TransformBounds()

I'm not sure what you mean here? We actually want to store the rectangular bounds of the clip here, that's the intention..

> 
> @@ +1785,5 @@
> > +            // XXX - FIX ME!!
> > +            MOZ_ASSERT(false);
> > +            gfxDebug() << "Source surface used for pattern too large!";
> > +            return NULL;
> > +          }
> 
> I think this hunk would be good to put in a function if it's not too hard.

I'll try.
 
> @@ +2051,5 @@
> >    }
> >  
> >    D2D1_FACTORY_OPTIONS options;
> >  #ifdef _DEBUG
> > +  options.debugLevel = D2D1_DEBUG_LEVEL_WARNING;
> 
> Intentional?

Yeah, INFORMATION gives a whole lot of spam which is rarely useful. WARNING should usually be relevant so it seemed to make more sense.

> 
> ::: gfx/2d/SourceSurfaceD2DTarget.cpp
> @@ +160,5 @@
> >      D2D1::BitmapProperties(D2D1::PixelFormat(DXGIFormat(mFormat), AlphaMode(mFormat)));
> >    hr = aRT->CreateSharedBitmap(IID_IDXGISurface, surf, &props, byRef(mBitmap));
> >  
> >    if (FAILED(hr)) {
> > +    // This seems to happen for FORMAT_A8 sometimes...
> 
> Did we run into this with the cairo backend too? We should try to use
> ReportError to find out more information about when it fails.

We did. We do more or less the same thing there. For me it seems to simply always happen when trying to create a shared bitmap for FORMAT_A8. I'm not sure about other hardware/etc. I think it's a Direct2D bug.
Comment 7 Bas Schouten (:bas.schouten) 2011-12-22 18:24:48 PST
Created attachment 583989 [details] [diff] [review]
Extend gfx::2d API v2

Updated to address review comments. Carrying r+ from jrmuizel.
Comment 8 Bas Schouten (:bas.schouten) 2011-12-22 18:26:18 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> (In reply to Bas Schouten (:bas) from comment #3)
> > I don't think so for DataSourceSurfaces? Not in my mind at least. There's a
> > good usecase for users that want to have an in-memory surface and then do
> > partial updates. Particularly in the case of small update areas it seems
> > wasteful to have to create a new SourceSurface each time.
> 
> Yes. However, we want to share SourceSurfaces across threads and that's much
> easier to do if they're guaranteed immutable. It's also difficult for people
> who want to hang onto references to cached SourceSurfaces if some are
> immutable and others aren't.

I didn't implement this yet in this patch. Since it isn't needed yet. I think it's a good idea but let's deal with it in a separate bug.
Comment 9 Bas Schouten (:bas.schouten) 2011-12-27 19:06:01 PST
Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c78b4872333
Comment 10 Bas Schouten (:bas.schouten) 2011-12-27 21:58:24 PST
Repushed to inbound after backout due to test failure - with some minor fixes r+'ed by roc on IRC.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e2e25fc3713
Comment 11 Matt Brubeck (:mbrubeck) 2011-12-28 11:10:34 PST
https://hg.mozilla.org/mozilla-central/rev/9c78b4872333
Comment 12 Matt Brubeck (:mbrubeck) 2011-12-28 11:12:12 PST
The backout and re-landing were also merged to m-c:
https://hg.mozilla.org/mozilla-central/rev/dc7c7734ec7c
https://hg.mozilla.org/mozilla-central/rev/6e2e25fc3713

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