Buffer Rotation requires allocating a new Gralloc surface

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

({perf})

Trunk
mozilla27
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:koi+, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [c=handeye p= s= u=1.2] )

Attachments

(1 attachment, 12 obsolete attachments)

Spin off from https://bugzilla.mozilla.org/show_bug.cgi?id=915120#c3.

This bug will track possible improvements for buffer rotation.
Once we have a solution and some numbers, let's consider for 26 uplift.
Posted patch patch - WIP - Passing gtest (obsolete) — Splinter Review
here's our first candidate for improving buffer rotation by swapping in-place. This isn't hooked up to the code yet.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Blocks: 915120
Keywords: perf
Whiteboard: [c= p= s= u=]
If we are hitting 1.1 perf without this, and we have confidence that tiled layers will make it and will not break HWC (the cases we care about, camera, video playback), we can probably punt on this for 1.2.
Won't this help with scroll performance in general? We do see stutters while scrolling for example in the SMS, Contacts and  E-mail apps.
Absolutely. It won't substantly increase average fps but will improve long frames aka scrolling bank.
bank/jank
Alright, lets finish it then. While we are killing buffer rotation, we are sadly not there yet. Also, I bet we can vectorize your code a bit.
I've fixed the bugs and my naive implementation already performs much better then current buffer unrotation. We go from 30ms to ~12ms for EndTransaction. I've got a few optimizations coming first because with ~12ms for rasterizing we still fall outside of our 15ms frame budget.
Alright I've got a templated version to optimize for 16/32. Runtime is in order of 4-8ms which moves the bottleneck to DrawBufferWithRotation. This is interesting because drawing the buffer is much easier then unrotating it so perhaps I can solve this here too.

This takes the Settings FPS to a *solid* 52-53 FPS between touch events without the nasty dips into ~40-30FPS.
Posted patch patch - WIP - needs cleanup (obsolete) — Splinter Review
Attachment #810815 - Attachment is obsolete: true
Posted patch patch v1 (obsolete) — Splinter Review
All done! Profiling with this patch is leading me to believe that hamachi vsync is 52Hz but I'm not sure how to confirm this.

With this fixed the other major source of us missing 52Hz is resizing the buffer requires us to allocate a new gralloc buffer.
Attachment #812823 - Attachment is obsolete: true
Attachment #812876 - Flags: review?(jmuizelaar)
Posted patch patch v1.1 (obsolete) — Splinter Review
Messed up when copying the patch from git to hg. Proper copy.
Attachment #812876 - Attachment is obsolete: true
Attachment #812876 - Flags: review?(jmuizelaar)
Attachment #812900 - Flags: review?(jmuizelaar)
Comment on attachment 812900 [details] [diff] [review]
patch v1.1

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

::: gfx/layers/BufferUnrotate.cpp
@@ +77,5 @@
> +{
> +  if ((aByteWidth%4) == 0 && (aByteWidth%4) == 0) {
> +    InplaceBufferUnrotate(reinterpret_cast<uint32_t*>(aBuffer), aByteWidth, aHeight, aByteStride, aXBoundary, aYBoundary);
> +  } else if ((aByteWidth%2) == 0 && (aByteWidth%2) == 0) {
> +    InplaceBufferUnrotate(reinterpret_cast<uint16_t*>(aBuffer), aByteWidth, aHeight, aByteStride, aXBoundary, aYBoundary);

I didn't do the unrolling yet. I could check if the buffer is a multiple of 8 bytes and have a template instantiation path for uint64_t. Even if we don't have any 64bit chips it should effectively let the compiler unroll the loop further.
With bug 907055 you should be able to confirm the max refresh rate.

Also, is really the gralloc buffer reallocation what causes us to miss that frame or repainting said new buffer? Because we could be smarter there if we want to and copy the old content into the new buffer and repaint only the enlarged area.
Comment on attachment 812900 [details] [diff] [review]
patch v1.1

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

::: gfx/layers/BufferUnrotate.cpp
@@ +74,5 @@
> +
> +void InplaceBufferUnrotate(unsigned char* aBuffer, int aByteWidth, int aHeight,
> +                           int aByteStride, int aXBoundary, int aYBoundary)
> +{
> +  if ((aByteWidth%4) == 0 && (aByteWidth%4) == 0) {

This condition makes no sense. You check twice and aByteWidth doesn't really correspond to whether the format is RBG16 or RGBA8888.
Attachment #812900 - Flags: review?(jmuizelaar) → review-
Opps the second should of been aByteStride. It doesn't matter what the format is since we're just moving bytes around. We can move RGB16 in chunks of 32 if it's a multiple of 32.
(In reply to Benoit Girard (:BenWa) from comment #16)
> Opps the second should of been aByteStride. It doesn't matter what the
> format is since we're just moving bytes around. We can move RGB16 in chunks
> of 32 if it's a multiple of 32.

The rotation may not be a multiple of 4 though.
For the y rotation it's fine since it's line based. For the x case we would need a swap the two pixels for rgba16 in the 32bit word.
(In reply to Benoit Girard (:BenWa) from comment #11)
> ...
> 
> All done! Profiling with this patch is leading me to believe that hamachi
> vsync is 52Hz but I'm not sure how to confirm this.

Michael, can you help us confirm or deny this finding?
Flags: needinfo?(mvines)
sync-1, can we have a information of vsync rate on hamachi/buri?
Flags: needinfo?(sync-1)
Flags: needinfo?(mvines)
hi Jack, can you help with comment 20? thanks
Flags: needinfo?(liuyongming)
Posted patch patch 1.2 (obsolete) — Splinter Review
I first added some 16 bit tests that failed because of what you pointed out, then fixed the problem.

I tried adding a uint64_t patch to unroll the loops but it didn't help on mac. I want to try it on ARM but it doesn't look promising.

I did an experiment compositing ASAP a blank frame and hamachi is stuck on 52Hz so we're hitting vsync or something preventing us from going above 52Hz. It would be great to confirm.

The buffer sync code is now the slowest piece while scrolling Settings but not enough to cause us to miss a frame. The low frequency gralloc allocate for buffer resize is now the primary source of missed frames while scrolling Settings however it isn't much.
Attachment #812900 - Attachment is obsolete: true
Attachment #813468 - Flags: review?(jmuizelaar)
Comment on attachment 813468 [details] [diff] [review]
patch 1.2

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

::: gfx/layers/BufferUnrotate.cpp
@@ +38,5 @@
> +        // Pre Condition:
> +        //   0..pivot is in the right place
> +        //   remaining..size is in increasing order
> +        //   pivot..remaining is rotated about nextStart
> +        Swap(&aBuffer[y*aByteStride/sizeof(T) + pivot], &aBuffer[y*aByteStride/sizeof(T) + remaining]);

A couple of performance comments that you can address in a follow up:
It's often faster to do pointer arithmetic directly. To do this well you'll want to change all
of remaining, pivot, and size to pointers to the same buffer.

You can also eliminate the multiplication by adding aByteStride/sizeof(T) to the appropriate pointer.

When doing this work make sure you watch the assembly to make sure that the changes are producing improvements.

::: gfx/layers/ThebesLayerBuffer.cpp
@@ +685,5 @@
>            mBufferRect = destBufferRect;
>          } else {
> +          if (IsAzureBuffer()) {
> +            RefPtr<SourceSurface> source = mDTBuffer->Snapshot();
> +            RefPtr<DataSourceSurface> dataSource = source->GetDataSurface();

I don't like the use of Snapshot() and GetDataSurface() here. Modifying a Snapshot() is not guaranteed to modify the buffer. I.e. think of what happens with a Direct2D draw target. We should only be doing an inplace rotate when we have direct access to the bits. You may need to do a bit of plumbing to hook this up properly. One possibility is to add API to the DrawTarget to borrow the raw bits and null otherwise.
Attachment #813468 - Flags: review?(jmuizelaar) → review-
Do you think the first part of the comment (optimization) can be done as a follow-up? Since the patch already as is provides a big improvement. The 2nd part sounds more serious but maybe we can live with that landed while BenWa works on a better API. The code here seems strictly better than what we have in tree, so I wonder whether incremental wins here.
Yes I'll do the first part in a follow up. The performance is good without this micro optimization because luckily the algorithm is relatively cache friendly. The second part is related to the try failure so I'll do it when I'm back in Toronto and will uplift this patch. Is koi? the flag I want? If not please fix it.
blocking-b2g: --- → koi?
Whats the patch you mentioned for the IPC gralloc issue that loses us frames?
blocking-b2g: koi? → leo+
Ohh I misunderstood. I don't have a patch for that. Nical and Sotaro are looking into this.
(In reply to Andreas Gal :gal from comment #27)
> Whats the patch you mentioned for the IPC gralloc issue that loses us frames?

That's bug 907309, but we're having a conversation as to what is still needed there.

Andreas, this is something you want us to consider uplifting to 1.1 with the leo+ flag?
Flags: needinfo?(gal)
Comment on attachment 813468 [details] [diff] [review]
patch 1.2

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

::: gfx/layers/BufferUnrotate.cpp
@@ +12,5 @@
> +{
> +  T tmpA = *a;
> +  *a = *b;
> +  *b = tmpA;
> +}

You're reinventing std::swap!
leo is done, all devices have been minted. We should only critical security fixes at this point.
Flags: needinfo?(gal)
Thanks - let's consider it for 1.2 then
blocking-b2g: leo+ → koi+
I just got a different hamachi device and this one is running past 60 FPS. I don't understand =\.
Posted patch patch v1.3 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&pusher=b56girard@gmail.com
Attachment #813468 - Attachment is obsolete: true
Attachment #815101 - Flags: review?
Comment on attachment 815101 [details] [diff] [review]
patch v1.3

Passing try. Bas can you review the GetLiveSourceSurface/ReleaseLiveSourceSurface bits?
Attachment #815101 - Flags: review? → review?(bas)
Comment on attachment 815101 [details] [diff] [review]
patch v1.3

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +426,5 @@
> +  cairo_content_t content = cairo_surface_get_content(mSurface);
> +  mLiveSurface = new SourceSurfaceCairo(mSurface,
> +                                        size,
> +                                        CairoContentToGfxFormat(content),
> +                                        this);

This still isn't going to do what you want if we can't get the bits out of mSurface directly.

Currently we only return the same bits if mSurface is a CAIRO_SURFACE_TYPE_IMAGE (see GetDataSurface - http://mxr.mozilla.org/mozilla-central/source/gfx/2d/SourceSurfaceCairo.cpp#61).

We could implement this for some other types, but it's not possible for all surface types (Xlib, win32 ddb, direct2d etc).
(In reply to Sotaro Ikeda [:sotaro] from comment #20)
> sync-1, can we have a information of vsync rate on hamachi/buri?

The vsync rate is about 62.
Flags: needinfo?(liuyongming)
Flags: needinfo?(sync-1)
Comment on attachment 815101 [details] [diff] [review]
patch v1.3

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

::: gfx/2d/2D.h
@@ +576,5 @@
> +  /**
> +   * Returns a live source surface to the contents of the DrawTarget if possible.
> +   * This is used for direct manipulation of underlying DrawTarget.
> +   */
> +  virtual TemporaryRef<SourceSurface> GetLiveSourceSurface() { return nullptr; };

SourceSurface is the wrong type to be exposing here. You just want to give access to the buffer. Something with void *data, width, height, stride. Perhaps even calling this LockBits() would work.
I tested rotation against a secondary surface and it's much slower. 5-8ms vs 10-12ms.
Posted patch patch v1.4 (obsolete) — Splinter Review
Like discussed I'll still test your approach but either way I will the LockBits.
Attachment #815101 - Attachment is obsolete: true
Attachment #815101 - Flags: review?(bas)
Attachment #815998 - Flags: review?(bas)
It seems we have a 62hz refresh. BenWa, any idea why we are not hitting that?
I received a new Hamachi with likely new blobs. I'm hitting 62hz no problem with the settings app minus the bugs I'm looking into.
Nice.
Blocks: 926267
Attachment #815998 - Attachment is obsolete: true
Attachment #815998 - Flags: review?(bas)
Attachment #817281 - Flags: review?(bas)
Comment on attachment 817281 [details] [diff] [review]
patch v2 (30% faster for horizontal rotation)

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

::: gfx/2d/2D.h
@@ +577,5 @@
> +   * If possible returns the bits to this DrawTarget for direct manipulation. While
> +   * the bits is locked any modifications to this DrawTarget is forbidden.
> +   * Release takes the original data pointer for safety.
> +   */
> +  virtual bool LockBits(unsigned char** aData, IntSize* aSize,

uint8_t

@@ +578,5 @@
> +   * the bits is locked any modifications to this DrawTarget is forbidden.
> +   * Release takes the original data pointer for safety.
> +   */
> +  virtual bool LockBits(unsigned char** aData, IntSize* aSize,
> +                        int32_t* aStride, SurfaceFormat* aFormat) { return false; };

nit: two stray semi-colons

@@ +579,5 @@
> +   * Release takes the original data pointer for safety.
> +   */
> +  virtual bool LockBits(unsigned char** aData, IntSize* aSize,
> +                        int32_t* aStride, SurfaceFormat* aFormat) { return false; };
> +  virtual void ReleaseBits(unsigned char* aData) {};

see above

::: gfx/2d/DrawTargetCairo.cpp
@@ +390,5 @@
>    if (mSurface) {
>      cairo_surface_destroy(mSurface);
>    }
> +#ifdef DEBUG
> +  if (mLockedBits) {

I believe we should be able to use MOZ_ASSERT which I believe internally does the same. This is part of MFBT and should be usable inside Moz2D.

@@ +423,5 @@
> +bool
> +DrawTargetCairo::LockBits(unsigned char** aData, IntSize* aSize,
> +                          int32_t* aStride, SurfaceFormat* aFormat)
> +{
> +  if (cairo_surface_get_type(mSurface) == CAIRO_SURFACE_TYPE_IMAGE) {

I wonder if we should support this for WIN32 DIBs as well.

@@ +1112,4 @@
>  DrawTargetCairo::WillChange(const Path* aPath /* = nullptr */)
>  {
>    MarkSnapshotIndependent();
> +#ifdef DEBUG

Same as above

::: gfx/2d/DrawTargetCairo.h
@@ +182,4 @@
>    cairo_surface_t* mSurface;
>    IntSize mSize;
>  
> +  unsigned char* mLockedBits;

uint8_t

::: gfx/layers/BufferUnrotate.cpp
@@ +14,5 @@
> +using mozilla::TimeStamp;
> +using mozilla::TimeDuration;
> +
> +template <class T> static
> +void SwapLine(T* a, T* b, int width)

This function can go away now! Yay!

@@ +25,5 @@
> +template <class T> static
> +void InplaceBufferUnrotateInternal(T* aBuffer, int aByteWidth, int aHeight,
> +                                  int aByteStride, int aXBoundary, int aYBoundary)
> +{
> +  if (aXBoundary != 0) {

I suspect the method used below will -still- be faster for x rotations, although perhaps less so. Did you happen to gather data on this?

@@ +37,5 @@
> +        // Pre Condition:
> +        //   0..pivot is in the right place
> +        //   remaining..size is in increasing order
> +        //   pivot..remaining is rotated about nextStart
> +        std::swap(aBuffer[y*aByteStride/sizeof(T) + pivot], aBuffer[y*aByteStride/sizeof(T) + remaining]);

nit: There's a bunch of places in this file with no spaces around operators. Please add them :).

@@ +50,5 @@
> +      }
> +    }
> +  }
> +
> +  // For horizontal buffer rotation it's about 30% to use memcpy between

This is technically actually a vertical rotation, the above code is the horizontal rotation.

@@ +51,5 @@
> +    }
> +  }
> +
> +  // For horizontal buffer rotation it's about 30% to use memcpy between
> +  // a temporary surface to use SIMD.

I don't really think SIMD has too much to do with it, I suspect it's simply because of the reduced memory traffic.

@@ +77,5 @@
> +
> +void InplaceBufferUnrotate(unsigned char* aBuffer, int aByteWidth, int aHeight,
> +                           int aByteStride, int aXBoundary, int aYBoundary)
> +{
> +  // NOTE: The uint64_t path is slightly slower on OSX64 so we don't use it

nit: I assume this comment is meant to say uint32_t? If we make this use memcpy for horizontal rotation as well I suspect we can just lose this additional complexity.

::: gfx/layers/ThebesLayerBuffer.cpp
@@ +684,5 @@
>            // Don't set destBuffer; we special-case self-copies, and
>            // just did the necessary work above.
>            mBufferRect = destBufferRect;
>          } else {
> +          // With azure and a data surface perform an inplace buffer unrotate

nit: This function could do with some vertical whitespace to make it a little easier to read.

@@ +692,5 @@
> +            IntSize size;
> +            int32_t stride;
> +            SurfaceFormat format;
> +            if (mDTBuffer->LockBits(&data, &size, &stride, &format)) {
> +              uint8_t bytesPerPixel = BytesPerPixel(format);

nit: Personally I'd just use uint32_t for this. Modern processors suck at 8 bit arithmetic.

::: gfx/tests/gtest/moz.build
@@ +21,4 @@
>      'TestRegion.cpp',
>      'TestColorNames.cpp',
>      'TestTextures.cpp',
> +    'TestBufferRotation.cpp',

Don't see the test file in this patch anymore.
Attachment #817281 - Flags: review?(bas) → review-
Whiteboard: [c= p= s= u=] → [c=handeye p= s= u=]
Blocks: 920921
(In reply to Bas Schouten (:bas.schouten) from comment #45)
> @@ +423,5 @@
> > +bool
> > +DrawTargetCairo::LockBits(unsigned char** aData, IntSize* aSize,
> > +                          int32_t* aStride, SurfaceFormat* aFormat)
> > +{
> > +  if (cairo_surface_get_type(mSurface) == CAIRO_SURFACE_TYPE_IMAGE) {
> 
> I wonder if we should support this for WIN32 DIBs as well.

I think so but I hope you don't mind if I let some else do it. I can add a TODO. I don't like adding code that wont be well tested.

> @@ +692,5 @@
> > +            IntSize size;
> > +            int32_t stride;
> > +            SurfaceFormat format;
> > +            if (mDTBuffer->LockBits(&data, &size, &stride, &format)) {
> > +              uint8_t bytesPerPixel = BytesPerPixel(format);
> 
> nit: Personally I'd just use uint32_t for this. Modern processors suck at 8
> bit arithmetic.

Switch what bit arithmetic? The buffer unrotation uses 16/32 as alignment allows. There isn't any bit arithmetic in this function.
Alright I just implemented the other unrotation with your approach and it seems to be 50% to 100% faster depending on the unrotation. With the latest changes the unrotation barely shows up in profiles, even less then before up to the point where I don't even care about it anymore:

http://people.mozilla.org/~bgirard/cleopatra/#report=505e033b49d34b0c5d64ccf337d23b0695712a4f&search=UNROTATE
Posted patch patch v2.1 (obsolete) — Splinter Review
Buffer unrotation barely shows up on Hamachi now :). Ship ship!
Attachment #817281 - Attachment is obsolete: true
Attachment #818091 - Flags: review?(bas)
Posted patch patch v2.2 (obsolete) — Splinter Review
+ missing file. I don't have the hang of sharing patches between hg (desktop) and git (mobile).
Attachment #818091 - Attachment is obsolete: true
Attachment #818091 - Flags: review?(bas)
Attachment #818109 - Flags: review?(bas)
Comment on attachment 818109 [details] [diff] [review]
patch v2.2

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +426,5 @@
> +    mLockedBits = cairo_image_surface_get_data(mSurface);
> +    *aData = mLockedBits;
> +    *aSize = GetSize();
> +    *aStride = cairo_image_surface_get_stride(mSurface);
> +    *aFormat = GetFormat();

Shouldn't the format of the surface be trivially retrievable? Do we have a reasonable use-case in mind for this being an outparam?

::: gfx/layers/BufferUnrotate.cpp
@@ +10,5 @@
> +#include <stdlib.h>
> +
> +void BufferUnrotate(uint8_t* aBuffer, int aByteWidth, int aHeight,
> +                    int aByteStride, int aXBoundary, int aYBoundary)
> +{

This code makes me much happier now :).

::: gfx/layers/BufferUnrotate.h
@@ +2,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +void BufferUnrotate(uint8_t* aBuffer, int aByteWidth, int aHeight,

Maybe we should just put the function in the header? It's pretty simple now? I don't care too much though, do what you prefer.

::: gfx/tests/gtest/TestBufferRotation.cpp
@@ +29,5 @@
> +  int xBoundary = 0;
> +  int yBoundary = 0;
> +  for (int y = 0; y < height; y++) {
> +    for (int x = 0; x < width; x++) {
> +     int pos = ((yBoundary+y) % height) * stride + ((xBoundary+x) % width) * bytesPerPixel;

nit: Maybe fix the spacing around operators here just for cleanness sake.
Attachment #818109 - Flags: review?(bas) → review+
(In reply to Bas Schouten (:bas.schouten) from comment #51)
> Comment on attachment 818109 [details] [diff] [review]
> patch v2.2
> 
> Review of attachment 818109 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetCairo.cpp
> @@ +426,5 @@
> > +    mLockedBits = cairo_image_surface_get_data(mSurface);
> > +    *aData = mLockedBits;
> > +    *aSize = GetSize();
> > +    *aStride = cairo_image_surface_get_stride(mSurface);
> > +    *aFormat = GetFormat();
> 
> Shouldn't the format of the surface be trivially retrievable? Do we have a
> reasonable use-case in mind for this being an outparam?

If we're returning lock bits I don't see how you would know for sure. You can query the format of the DrawTarget and assume that they're the same but I don't see the harm of doing it explicitly. Otherwise you can infer some information based on stride/width but it's still potentially ambiguous.

> ::: gfx/layers/BufferUnrotate.h
> @@ +2,5 @@
> > + * This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +void BufferUnrotate(uint8_t* aBuffer, int aByteWidth, int aHeight,
> 
> Maybe we should just put the function in the header? It's pretty simple now?
> I don't care too much though, do what you prefer.

As a static? I'm not a fan of having non trivial static function in headers.
Posted patch patch v2.3 (obsolete) — Splinter Review
Attachment #818109 - Attachment is obsolete: true
Attachment #818681 - Flags: review+
Posted patch patch v2.4 (obsolete) — Splinter Review
Fix windows build failure, waiting on CLOSED TREE to push
Attachment #818681 - Attachment is obsolete: true
Attachment #818740 - Flags: review+
Inbound is still closed, requesting checkin-needed
Keywords: checkin-needed
Posted patch patch v2.5Splinter Review
I forgot trychooser schedules opt only by default. Fixed the problem locally and doing another run with debug enabled.
https://tbpl.mozilla.org/?tree=Try&rev=0c5d5b59bd02
Attachment #818740 - Attachment is obsolete: true
Attachment #819043 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/27b3b090e4fd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 929439
This will need an updated patch for aurora/koi uplift:

[/c/src-gecko/aurora]$ transplant 27b3b090e4fd
searching for changes
applying 27b3b090e4fd
patching file gfx/2d/DrawTargetCairo.cpp
Hunk #1 FAILED at 393
Hunk #3 FAILED at 1171
2 out of 3 hunks FAILED -- saving rejects to file gfx/2d/DrawTargetCairo.cpp.rej
patching file gfx/2d/DrawTargetCairo.h
Hunk #1 succeeded at 54 with fuzz 1 (offset -2 lines).
Hunk #2 FAILED at 183
1 out of 2 hunks FAILED -- saving rejects to file gfx/2d/DrawTargetCairo.h.rej
patching file gfx/layers/moz.build
Hunk #1 FAILED at 188
1 out of 1 hunks FAILED -- saving rejects to file gfx/layers/moz.build.rej
patching file gfx/tests/gtest/moz.build
Hunk #1 FAILED at 15
1 out of 1 hunks FAILED -- saving rejects to file gfx/tests/gtest/moz.build.rej
patch failed to apply
Whiteboard: [c=handeye p= s= u=] → [c=handeye p= s= u=] [needs-aurora-patch]
Attempt at unbitrotting:

https://hg.mozilla.org/releases/mozilla-aurora/rev/77d48a04eef0
Whiteboard: [c=handeye p= s= u=] [needs-aurora-patch] → [c=handeye p= s= u=]
Had to push a follow-up to fix a boneheaded mistake on my part, but it looks like it's going to stick :)
https://hg.mozilla.org/releases/mozilla-aurora/rev/a7cd08f87a54
(In reply to Ryan VanderMeulen [:RyanVM UTC-4][PTO 10/19 - 10/28] from comment #65)
> Backed out for suspicion of causing Android reftest failures.

Confirmed that backing out fixed the failures.
Gah, comments 65 and 66 were intended for bug 920160, not this bug. Sorry for that!
Flags: needinfo?(bgirard)
Whiteboard: [c=handeye p= s= u=] → [c=handeye p= s= u=1.2]
You need to log in before you can comment on or make changes to this bug.