921212 - Buffer Rotation requires allocating a new Gralloc surface

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

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

This bug will track possible improvements for buffer rotation.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Once we have a solution and some numbers, let's consider for 26 uplift.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch - WIP - Passing gtest

here's our first candidate for improving buffer rotation by swapping in-place. This isn't hooked up to the code yet.

Comment 3

[Andreas Gal :gal]

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.

Comment 4

[Mandyam Vikram]

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.

Comment 5

[Benoit Girard (:BenWa)]

Absolutely. It won't substantly increase average fps but will improve long frames aka scrolling bank.

Comment 6

[Benoit Girard (:BenWa)]

bank/jank

Comment 7

[Andreas Gal :gal]

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.

Comment 8

[Benoit Girard (:BenWa)]

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.

Comment 9

[Benoit Girard (:BenWa)]

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.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch - WIP - needs cleanup

Comment 11

[Benoit Girard (:BenWa)]

Attachment: patch v1

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.

Comment 12

[Benoit Girard (:BenWa)]

Attachment: patch v1.1

Messed up when copying the patch from git to hg. Proper copy.

Comment 13

[Benoit Girard (:BenWa)]

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.

Comment 14

[Andreas Gal :gal]

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 15

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 16

[Benoit Girard (:BenWa)]

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.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 18

[Benoit Girard (:BenWa)]

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.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

(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?

Comment 20

[Sotaro Ikeda [:sotaro]]

sync-1, can we have a information of vsync rate on hamachi/buri?

Comment 21

[Joe Cheng [:jcheng] (please needinfo)]

hi Jack, can you help with comment 20? thanks

Comment 22

[Benoit Girard (:BenWa)]

Attachment: patch 1.2

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.

Comment 23

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=27350c008bae

Comment 24

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 25

[Andreas Gal :gal]

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.

Comment 26

[Benoit Girard (:BenWa)]

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.

Comment 27

[Andreas Gal :gal]

Whats the patch you mentioned for the IPC gralloc issue that loses us frames?

Comment 28

[Benoit Girard (:BenWa)]

Ohh I misunderstood. I don't have a patch for that. Nical and Sotaro are looking into this.

Comment 29

[Milan Sreckovic [:milan] (needinfo for best results)]

(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?

Comment 30

[(no longer active)]

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!

Comment 31

[Andreas Gal :gal]

leo is done, all devices have been minted. We should only critical security fixes at this point.

Comment 32

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks - let's consider it for 1.2 then

Comment 33

[Benoit Girard (:BenWa)]

I just got a different hamachi device and this one is running past 60 FPS. I don't understand =\.

Comment 34

[Benoit Girard (:BenWa)]

Attachment: patch v1.3

https://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]

Comment 35

[Benoit Girard (:BenWa)]

Comment on attachment 815101 [details] [diff] [review]
patch v1.3

Passing try. Bas can you review the GetLiveSourceSurface/ReleaseLiveSourceSurface bits?

Comment 36

[Matt Woodrow (:mattwoodrow)]

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).

Comment 37

[Jack Liu]

(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.

Comment 38

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 39

[Benoit Girard (:BenWa)]

I tested rotation against a secondary surface and it's much slower. 5-8ms vs 10-12ms.

Comment 40

[Benoit Girard (:BenWa)]

Attachment: patch v1.4

Like discussed I'll still test your approach but either way I will the LockBits.

Comment 41

[Andreas Gal :gal]

It seems we have a 62hz refresh. BenWa, any idea why we are not hitting that?

Comment 42

[Benoit Girard (:BenWa)]

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.

Comment 43

[Andreas Gal :gal]

Nice.

Comment 44

[Benoit Girard (:BenWa)]

Attachment: patch v2 (30% faster for horizontal rotation)

Comment 45

[Bas Schouten (:bas.schouten)]

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.

Comment 46

[Benoit Girard (:BenWa)]

(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.

Comment 47

[Benoit Girard (:BenWa)]

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

Comment 48

[Benoit Girard (:BenWa)]

Attachment: patch v2.1

Buffer unrotation barely shows up on Hamachi now :). Ship ship!

Comment 49

[Benoit Girard (:BenWa)]

Attachment: patch v2.2

+ missing file. I don't have the hang of sharing patches between hg (desktop) and git (mobile).

Comment 50

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=245c306fc804

Comment 51

[Bas Schouten (:bas.schouten)]

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.

Comment 52

[Benoit Girard (:BenWa)]

(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.

Comment 53

[Benoit Girard (:BenWa)]

Attachment: patch v2.3

Comment 54

[Benoit Girard (:BenWa)]

Attachment: patch v2.4

Fix windows build failure, waiting on CLOSED TREE to push

Comment 55

[Benoit Girard (:BenWa)]

Inbound is still closed, requesting checkin-needed

Comment 56

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6150269410b2

Comment 57

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ae8ea90aff4

https://tbpl.mozilla.org/php/getParsedLog.php?id=29311956&tree=Mozilla-Inbound

Comment 58

[Benoit Girard (:BenWa)]

Attachment: patch v2.5

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

Comment 59

[Benoit Girard (:BenWa)]

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

Comment 60

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27b3b090e4fd

Comment 61

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

https://hg.mozilla.org/mozilla-central/rev/27b3b090e4fd

Comment 62

[Ed Morley [:emorley]]

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

Comment 63

[Ryan VanderMeulen [:RyanVM]]

Attempt at unbitrotting:

https://hg.mozilla.org/releases/mozilla-aurora/rev/77d48a04eef0

Comment 64

[Ryan VanderMeulen [:RyanVM]]

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

Comment 65

[Ryan VanderMeulen [:RyanVM]]

Backed out for suspicion of causing Android reftest failures.
https://hg.mozilla.org/releases/mozilla-aurora/rev/7090e4028a66

https://tbpl.mozilla.org/php/getParsedLog.php?id=29752631&tree=Mozilla-Aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=29752478&tree=Mozilla-Aurora

Comment 66

[Ryan VanderMeulen [:RyanVM]]

(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.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

Gah, comments 65 and 66 were intended for bug 920160, not this bug. Sorry for that!