Closed Bug 702878 Opened 13 years ago Closed 12 years ago

[Azure] Extend gfx::2d API for Azure Thebes wrapper

Categories

(Core :: Graphics, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Attached patch Extend gfx::2d API (obsolete) — Splinter Review
This patch has the additions to the API and the implementation for the Direct2D backend.
Attachment #574917 - Flags: superreview?(roc)
Attachment #574917 - Flags: review?(jmuizelaar)
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.
(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.
(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 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.
Attachment #574917 - Flags: review?(jmuizelaar) → review+
(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.
Updated to address review comments. Carrying r+ from jrmuizel.
Attachment #574917 - Attachment is obsolete: true
Attachment #574917 - Flags: superreview?(roc)
Attachment #583989 - Flags: superreview?(roc)
Attachment #583989 - Flags: review+
(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.
Attachment #583989 - Flags: superreview?(roc) → superreview+
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
https://hg.mozilla.org/mozilla-central/rev/9c78b4872333
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.