740580 - [Azure] Add support for Azure to Layers

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[George Wright (:gw280) (needinfo me!)]

We should add support for DrawTargets to Layers for content rendering.

Comment 1

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 740580 - Add support for Azure DrawTargets to BasicLayers

This patch is based heavily on work done by Matt Woodrow. Putting up for review to see if
this is going down the right direction or if we want to do something else such as create
an AzureBasicLayers (or similar) class instead.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 610685 [details] [diff] [review]
Bug 740580 - Add support for Azure DrawTargets to BasicLayers

Sorry, I don't know this code well enough to review. Bas?

Comment 3

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 740580 - Add support for Azure DrawTargets to BasicLayers

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 616203 [details] [diff] [review]
Bug 740580 - Add support for Azure DrawTargets to BasicLayers

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

This is ok for now I guess. On the longer term we just want an Azure version of BasicLayers.

Comment 5

[George Wright (:gw280) (needinfo me!)]

Attachment: Expose SHM info

Comment 6

[George Wright (:gw280) (needinfo me!)]

Attachment: Updated basic layers patch

Comment 7

[Bas Schouten (:bas.schouten)]

Comment on attachment 631046 [details] [diff] [review]
Updated basic layers patch

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

This polymorphism is pretty terrible, but it'll have to do for now!

::: gfx/layers/ThebesLayerBuffer.h
@@ +139,5 @@
> +   */
> +  TemporaryRef<gfx::DrawTarget>
> +  CreateDrawTarget(const nsIntSize& aSize, ContentType aContent) 
> +  {
> +    gfx::SurfaceFormat format = gfx::SurfaceFormatForImageFormat(gfxPlatform::GetPlatform()->OptimalFormatForContent(aContent));

Line break here somewhere, I'm not too picky on the 80 character limit but this is a bit long.

@@ +193,5 @@
>    }
> +  
> +  TemporaryRef<gfx::DrawTarget>
> +  SetDT(gfx::DrawTarget* aBuffer,
> +            const nsIntRect& aBufferRect, const nsIntPoint& aBufferRotation)

nit: indent

Comment 8

[George Wright (:gw280) (needinfo me!)]

I gave up waiting for autoland:

https://hg.mozilla.org/try/rev/06471da82ec1

Comment 9

[Mozilla RelEng Bot]

Try run for 5de68d27f6ca is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5de68d27f6ca
Results (out of 251 total builds):
    exception: 1
    success: 228
    warnings: 18
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5de68d27f6ca

Comment 10

[Mozilla RelEng Bot]

Try run for 5de68d27f6ca is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5de68d27f6ca
Results (out of 259 total builds):
    exception: 2
    success: 231
    warnings: 22
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5de68d27f6ca

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 1: Expose SHM info v2 (rebased)

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Add DrawTarget support BasicLayer v4 (rebased)

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 3: Add DrawTarget support to CanvasFrame

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://tbpl.mozilla.org/?tree=Try&rev=44be6782af47

Comment 15

[Mozilla RelEng Bot]

Try run for 194f95c1f3e7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=194f95c1f3e7
Results (out of 50 total builds):
    exception: 1
    success: 43
    warnings: 5
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-194f95c1f3e7

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Add DrawTarget support BasicLayer v5 (rebased)

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Add DrawTarget support BasicLayer v6

Remove UseAzureContentDrawing() in favour of SupportsAzureContent(). See Bug 793085.

Comment 18

[Mozilla RelEng Bot]

Try run for c4c1e60a3aac is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c4c1e60a3aac
Results (out of 213 total builds):
    exception: 5
    success: 191
    warnings: 13
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c4c1e60a3aac

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Add DrawTarget support BasicLayer v7

Comment 20

[Bas Schouten (:bas.schouten)]

Comment on attachment 658376 [details] [diff] [review]
Part 3: Add DrawTarget support to CanvasFrame

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

I'm not the right person to review this I think.

Comment 21

[Mozilla RelEng Bot]

Try run for 0e1e1c0046b5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0e1e1c0046b5
Results (out of 14 total builds):
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0e1e1c0046b5

Comment 22

[Bas Schouten (:bas.schouten)]

Comment on attachment 664316 [details] [diff] [review]
Part 2: Add DrawTarget support BasicLayer v7

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

::: gfx/layers/ThebesLayerBuffer.cpp
@@ +319,5 @@
>            nsIntPoint dest = mBufferRect.TopLeft() - destBufferRect.TopLeft();
> +          EnsureBuffer();
> +          if (!mBuffer)
> +            return result;
> +          mBuffer->MovePixels(srcRect, dest);

Eventually we want to at least support self-copy through snapshot & draw here.

::: gfx/layers/ThebesLayerBuffer.h
@@ +136,5 @@
> +  CreateDrawTarget(const gfx::IntSize& aSize, gfx::SurfaceFormat aFormat)
> +  {
> +    return gfxPlatform::GetPlatform()->CreateOffscreenDrawTarget(aSize, aFormat);
> +  }
> +  

nit: whitespace

@@ +143,5 @@
> +   */
> +  TemporaryRef<gfx::DrawTarget>
> +  CreateDrawTarget(const nsIntSize& aSize, ContentType aContent) 
> +  {
> +    gfx::SurfaceFormat format = gfx::SurfaceFormatForImageFormat(gfxPlatform::GetPlatform()->OptimalFormatForContent(aContent));

nit: split this over more lines

@@ +194,5 @@
>      mBufferRect = aBufferRect;
>      mBufferRotation = aBufferRotation;
>      return tmp.forget();
>    }
> +  

nit: whitespace

::: gfx/layers/basic/BasicThebesLayer.cpp
@@ +519,5 @@
> +      enum { buflen = 256 };
> +      char buf[buflen];
> +      PR_snprintf(buf, buflen,
> +                  "creating ThebesLayer 'back buffer' failed! width=%d, height=%d, type=%x",
> +                  aSize.width, aSize.height, int(mozilla::gfx::ContentForFormat(aFormat)));

Please don't do printfs...

@@ +528,5 @@
> +                    "Bad! Did we create a buffer twice without painting?");
> +
> +  mIsNewBuffer = true;
> +
> +  mozilla::ipc::Shmem shm = mBackBuffer.get_Shmem();

We should probably error check some here.

::: gfx/layers/basic/BasicThebesLayer.h
@@ +11,3 @@
>  #include "BasicBuffers.h"
>  
> +using namespace mozilla::gfx;

No using namespace inside headers, you can typedef inside the class definition if you use a class a lot.

@@ +49,5 @@
>                             void* aCallbackData,
>                             ReadbackProcessor* aReadback);
>  
>    virtual void ClearCachedResources() { mBuffer.Clear(); mValidRegion.SetEmpty(); }
>    

nit: whitespace

::: gfx/layers/ipc/AutoOpenSurface.h
@@ +15,4 @@
>  #include "mozilla/layers/PLayers.h"
>  #include "ShadowLayers.h"
>  
> +using namespace mozilla::gfx;

idem

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +493,5 @@
> +  switch (aSurface.type()) {
> +  case SurfaceDescriptor::TShmem: {
> +    mozilla::ipc::Shmem shm = aSurface.get_Shmem();
> +    SharedImageInfo* shmInfo = gfxSharedImageSurface::GetShmInfoPtr(shm);
> +    unsigned char* data = shm.get<unsigned char>();

Some error checking we should probably do here.

Comment 23

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Add DrawTarget support BasicLayer v8

Comment 24

[Mozilla RelEng Bot]

Try run for 560ee99a5fef is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=560ee99a5fef
Results (out of 93 total builds):
    success: 72
    warnings: 21
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-560ee99a5fef

Comment 25

[Mozilla RelEng Bot]

Try run for b1ad5229d516 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b1ad5229d516
Results (out of 90 total builds):
    success: 77
    warnings: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b1ad5229d516

Comment 26

[Mozilla RelEng Bot]

Try run for b1ad5229d516 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b1ad5229d516
Results (out of 97 total builds):
    success: 83
    warnings: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b1ad5229d516

Comment 27

[Mozilla RelEng Bot]

Try run for b1ad5229d516 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b1ad5229d516
Results (out of 98 total builds):
    success: 83
    warnings: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b1ad5229d516

Comment 28

[Nick Cameron [:nrc]]

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #23)
> Created attachment 665771 [details] [diff] [review]
> Part 2: Add DrawTarget support BasicLayer v8

Did you mean to ask someone specific to review this? It is very unlikey to get reviewed without asking for a reviewer :-)

Comment 29

[Bas Schouten (:bas.schouten)]

Comment on attachment 665771 [details] [diff] [review]
Part 2: Add DrawTarget support BasicLayer v8

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

::: gfx/layers/basic/BasicBuffers.cpp
@@ +108,5 @@
>    BasicThebesLayerBuffer srcBuffer(aSource, aRect, aRotation);
>    srcBuffer.DrawBufferWithRotation(destCtx, 1.0);
>  }
>  
> +

nit: whitespace

::: gfx/layers/basic/BasicBuffers.h
@@ +9,5 @@
>  #include "BasicLayersImpl.h"
>  
> +#include "mozilla/gfx/2D.h"
> +
> +using namespace mozilla::gfx;

No using namespace in headers please!

::: gfx/layers/basic/BasicThebesLayer.h
@@ +51,5 @@
>    
>    virtual already_AddRefed<gfxASurface>
>    CreateBuffer(Buffer::ContentType aType, const nsIntSize& aSize);
>  
> +  virtual TemporaryRef<DrawTarget>

I think this relies on BasicBuffers to pull in the namespace gfx.. bad!

Comment 30

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Add DrawTarget support BasicLayer v9

Fixed name space problem as requested.

Comment 31

[Mozilla RelEng Bot]

Try run for b1ad5229d516 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b1ad5229d516
Results (out of 99 total builds):
    success: 84
    warnings: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b1ad5229d516

Comment 32

[Mozilla RelEng Bot]

Try run for 42ab4ac92782 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=42ab4ac92782
Results (out of 93 total builds):
    success: 83
    warnings: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-42ab4ac92782

Comment 33

[Mozilla RelEng Bot]

Try run for 42ab4ac92782 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=42ab4ac92782
Results (out of 94 total builds):
    success: 83
    warnings: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-42ab4ac92782

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd12dcf7f8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff5e81e6de6
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae09da96f63

Should this have tests?

Comment 35

[George Wright (:gw280) (needinfo me!)]

Shouldn't do; this enables a code-path that is preffed off for now, and the existing test suite is sufficient in terms of coverage.

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0cd12dcf7f8f
https://hg.mozilla.org/mozilla-central/rev/5ff5e81e6de6
https://hg.mozilla.org/mozilla-central/rev/0ae09da96f63

Comment 37

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Hey guys, PRInt32 is deprecated, you should use int32_t instead. Please keep a closer eye on those in the future?

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 797391.
https://hg.mozilla.org/mozilla-central/rev/0a095af171f4

So, about those tests? :-). Also, please note Ms2ger's comment in your next revision.

Comment 39

[Mozilla RelEng Bot]

Try run for e0757d18462a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e0757d18462a
Results (out of 6 total builds):
    failure: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e0757d18462a

Comment 40

[George Wright (:gw280) (needinfo me!)]

A conversation from IRC to remind me what needs to be done, as I'll be taking over ownership of landing this again:

03:14:41 < kentuckyfriedtakahe> so my plan was to land 793013.
03:14:55 < kentuckyfriedtakahe> then to land 800690.
03:15:20 < kentuckyfriedtakahe> so 793013 just has some annoying break on windows.
03:16:56 < kentuckyfriedtakahe> so it's a bit of a pain.
03:17:47 < kentuckyfriedtakahe> but I think 750434 is ready to go and so is 740580
03:19:06 < kentuckyfriedtakahe> hmm.. 740580 part II needs something.

Comment 41

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d08cc0f8212b

Just landed part 3 as its the only one that is still required.

Comment 42

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2f676bad38a

Comment 43

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d08cc0f8212b
https://hg.mozilla.org/mozilla-central/rev/c2f676bad38a