732988 - Make NPAPI Async plugin drawing scale correctly

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add ScaleToSize API to ImageLayer

The NPAPI async spec says plugin surfaces should be scaled to cover the plugin child area.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Scale plugin surfaces to client area size

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 602921 [details] [diff] [review]
Part 1: Add ScaleToSize API to ImageLayer

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

::: gfx/layers/ImageLayers.h
@@ +583,5 @@
>  public:
> +  enum ScaleMode {
> +    SCALE_NONE,
> +    SCALE_STRETCH,
> +    SCALE_PRESERVE_ASPECT_RATIO_CONTAIN

Let's not add this (or comment it out) until it's supported.

And add a comment that STRETCH is not handled on GL or e10s yet.

@@ +607,5 @@
> +   * Set the size to scale the image to and the mode at which to scale.
> +   */
> +  void SetScaleToSize(const gfxIntSize &aSize, ScaleMode aMode)
> +  {
> +    mSize = aSize; mScaleMode = aMode;

two lines

@@ +645,3 @@
>    nsRefPtr<ImageContainer> mContainer;
>    gfxPattern::GraphicsFilter mFilter;
> +  gfxIntSize mSize;

Better call this "mScaleToSize" to avoid confusion. It's not the size of the image (UNLIKE mSize in BasicImageLayer).

::: gfx/layers/basic/BasicLayers.cpp
@@ -848,5 @@
>  class BasicImageLayer : public ImageLayer, public BasicImplData {
>  public:
>    BasicImageLayer(BasicLayerManager* aLayerManager) :
> -    ImageLayer(aLayerManager, static_cast<BasicImplData*>(this)),
> -    mSize(-1, -1)

You're removing this, but BasicShadowableImageLayer::Paint is still using it and ends up using the mSize in ImageLayer which is wrong!

@@ +929,5 @@
>    // The visible region can extend outside the image.  If we're not
>    // tiling, we don't want to draw into that area, so just draw within
>    // the image bounds.
>    const nsIntRect* tileSrcRect = GetTileSourceRect();
> +  AutoSetOperator setOpera7tor(aContext, GetOperator());

fix typo!

::: gfx/layers/d3d10/ImageLayerD3D10.cpp
@@ +129,5 @@
>    if (!image) {
>      return;
>    }
>  
> +  gfxIntSize size = mScaleMode == SCALE_NONE ?  image->GetSize() : mSize;

only one space after ?

::: gfx/layers/d3d9/ImageLayerD3D9.cpp
@@ +324,5 @@
>    }
>  
>    SetShaderTransformAndOpacity();
>  
> +  gfxIntSize size = mScaleMode == SCALE_NONE ?  image->GetSize() : mSize;

only one space after ?

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add ScaleToSize API to ImageLayer v2

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 604424 [details] [diff] [review]
Part 1: Add ScaleToSize API to ImageLayer v2

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

::: gfx/layers/ImageLayers.h
@@ +646,3 @@
>    nsRefPtr<ImageContainer> mContainer;
>    gfxPattern::GraphicsFilter mFilter;
> +  gfxIntSize mScaleToSize;

Comment that this is only valid of mScaleMode != SCALE_NONE.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We need ShadowLayers support for this... who's going to do it and when?

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #5)
> We need ShadowLayers support for this... who's going to do it and when?

It's a good question, I guess right now we don't use shadowlayers on any platform which supports it. But we will need it in the future. Benoit Girard might be a good person as he works on OMTC and might also work on the accelerated drawing model for Mac so will need to get in touch with some of the new plugin drawing code.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess one big question is who's going to do OMTC for Windows.

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Scale plugin surfaces to client area size

Updated to work with Mac drawing models as per e-mail discussion.

Comment 9

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b46f6eff8f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/f667f6c22bed

Comment 10

[Daniel Holbert [:dholbert]]

Backed out:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/7bbff0b46460
  https://hg.mozilla.org/integration/mozilla-inbound/rev/52e11f6c80b9
for having apparently caused a Linux opt Cipc crashtest failure.

Sample logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=9990114&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=9990076&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=9989912&tree=Mozilla-Inbound

The failing test is:
  http://mxr.mozilla.org/mozilla-central/source/layout/base/crashtests/50395-1.html?force=1

The crash is us hitting the final NS_RUNTIMEABORT() in BasicShadowableImageLayer::Paint and aborting.

Comment 11

[Daniel Holbert [:dholbert]]

csets for landing & backout merged to m-c.

Landing:
 https://hg.mozilla.org/mozilla-central/rev/4b46f6eff8f4
 https://hg.mozilla.org/mozilla-central/rev/f667f6c22bed
Backout:
 https://hg.mozilla.org/mozilla-central/rev/7bbff0b46460
 https://hg.mozilla.org/mozilla-central/rev/52e11f6c80b9

Comment 12

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add ScaleToSize API to ImageLayer v3

The old patch had an issue with shadowableImageLayer where mSize would not be updated correctly. This patch fixes that.

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/709bce4da141
https://hg.mozilla.org/mozilla-central/rev/babf5e9f6036