Last Comment Bug 732988 - Make NPAPI Async plugin drawing scale correctly
: Make NPAPI Async plugin drawing scale correctly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-05 08:23 PST by Bas Schouten (:bas.schouten)
Modified: 2012-03-13 05:55 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add ScaleToSize API to ImageLayer (12.08 KB, patch)
2012-03-05 08:23 PST, Bas Schouten (:bas.schouten)
roc: review-
Details | Diff | Review
Part 2: Scale plugin surfaces to client area size (1.55 KB, patch)
2012-03-05 08:24 PST, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Review
Part 1: Add ScaleToSize API to ImageLayer v2 (12.94 KB, patch)
2012-03-09 08:43 PST, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Review
Part 2: Scale plugin surfaces to client area size (1.80 KB, patch)
2012-03-11 09:09 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Review
Part 1: Add ScaleToSize API to ImageLayer v3 (10.89 KB, patch)
2012-03-12 12:07 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Review

Description Bas Schouten (:bas.schouten) 2012-03-05 08:23:41 PST
Created attachment 602921 [details] [diff] [review]
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) 2012-03-05 08:24:09 PST
Created attachment 602922 [details] [diff] [review]
Part 2: Scale plugin surfaces to client area size
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-06 15:02:50 PST
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) 2012-03-09 08:43:21 PST
Created attachment 604424 [details] [diff] [review]
Part 1: Add ScaleToSize API to ImageLayer v2
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-09 08:48:28 PST
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) (Exited; email my personal email if necessary) 2012-03-09 08:49:26 PST
We need ShadowLayers support for this... who's going to do it and when?
Comment 6 Bas Schouten (:bas.schouten) 2012-03-09 09:02:43 PST
(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) (Exited; email my personal email if necessary) 2012-03-10 13:11:21 PST
I guess one big question is who's going to do OMTC for Windows.
Comment 8 Bas Schouten (:bas.schouten) 2012-03-11 09:09:04 PDT
Created attachment 604776 [details] [diff] [review]
Part 2: Scale plugin surfaces to client area size

Updated to work with Mac drawing models as per e-mail discussion.
Comment 12 Bas Schouten (:bas.schouten) 2012-03-12 12:07:55 PDT
Created attachment 605043 [details] [diff] [review]
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.

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