[Azure] Add support for Azure to Layers

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: gw280, Assigned: gw280)

Tracking

unspecified
mozilla26
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 9 obsolete attachments)

2.18 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
2.88 KB, patch
roc
: review+
Details | Diff | Splinter Review
36.78 KB, patch
Details | Diff | Splinter Review
We should add support for DrawTargets to Layers for content rendering.
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.
Attachment #610685 - Flags: review?(bgirard)
Blocks: 740200
Depends on: 740598, 740570
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?
Attachment #610685 - Flags: review?(bgirard) → review?(bas.schouten)
Attachment #610685 - Attachment is obsolete: true
Attachment #610685 - Flags: review?(bas.schouten)
Attachment #616203 - Flags: review?(bas.schouten)
Depends on: 746588
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.
Attachment #616203 - Flags: review?(bas.schouten) → review+
Blocks: 725119
Posted patch Expose SHM info (obsolete) — Splinter Review
Attachment #631045 - Flags: review?(bas.schouten)
Posted patch Updated basic layers patch (obsolete) — Splinter Review
Attachment #616203 - Attachment is obsolete: true
Attachment #631046 - Flags: review?(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
Attachment #631046 - Flags: review?(bas.schouten) → review+
Attachment #631045 - Flags: review?(bas.schouten) → review+
Whiteboard: [autoland-try:631045,631046]
I gave up waiting for autoland:

https://hg.mozilla.org/try/rev/06471da82ec1
Whiteboard: [autoland-try:631045,631046]
Blocks: 750434
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/ajones@mozilla.com-5de68d27f6ca
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/ajones@mozilla.com-5de68d27f6ca
Assignee: nobody → ajones
Status: NEW → ASSIGNED
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/ajones@mozilla.com-194f95c1f3e7
Attachment #658375 - Attachment is obsolete: true
Attachment #658375 - Flags: review?(bas.schouten)
Attachment #661042 - Flags: review?
Attachment #661042 - Flags: review? → review?(bas.schouten)
Remove UseAzureContentDrawing() in favour of SupportsAzureContent(). See Bug 793085.
Attachment #661042 - Attachment is obsolete: true
Attachment #661042 - Flags: review?(bas.schouten)
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/ajones@mozilla.com-c4c1e60a3aac
Attachment #663282 - Attachment is obsolete: true
Attachment #664316 - Flags: review?(bas.schouten)
Attachment #658374 - Flags: review?(bas.schouten) → review+
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.
Attachment #658376 - Flags: review?(bas.schouten) → review?(roc)
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/ajones@mozilla.com-0e1e1c0046b5
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.
Attachment #664316 - Flags: review?(bas.schouten) → review-
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/ajones@mozilla.com-560ee99a5fef
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/ajones@mozilla.com-b1ad5229d516
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/ajones@mozilla.com-b1ad5229d516
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/ajones@mozilla.com-b1ad5229d516
(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 :-)
Attachment #665771 - Flags: review? → review?(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!
Attachment #665771 - Flags: review?(bas.schouten) → review+
Fixed name space problem as requested.
Attachment #665771 - Attachment is obsolete: true
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/ajones@mozilla.com-b1ad5229d516
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/ajones@mozilla.com-42ab4ac92782
Keywords: checkin-needed
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/ajones@mozilla.com-42ab4ac92782
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.
https://hg.mozilla.org/mozilla-central/rev/0cd12dcf7f8f
https://hg.mozilla.org/mozilla-central/rev/5ff5e81e6de6
https://hg.mozilla.org/mozilla-central/rev/0ae09da96f63
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Flags: in-testsuite? → in-testsuite-
Hey guys, PRInt32 is deprecated, you should use int32_t instead. Please keep a closer eye on those in the future?
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.
Status: RESOLVED → REOPENED
Flags: in-testsuite- → in-testsuite?
Resolution: FIXED → ---
Target Milestone: mozilla18 → ---
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/ajones@mozilla.com-e0757d18462a
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.
Assignee: ajones → gwright
Depends on: 880763
https://hg.mozilla.org/integration/mozilla-inbound/rev/d08cc0f8212b

Just landed part 3 as its the only one that is still required.
https://hg.mozilla.org/mozilla-central/rev/d08cc0f8212b
https://hg.mozilla.org/mozilla-central/rev/c2f676bad38a
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Updated

6 years ago
Duplicate of this bug: 909238
You need to log in before you can comment on or make changes to this bug.