Benoit Girard (:BenWa) Assignee
	Description • 11 years ago

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

This bug will track possible improvements for buffer rotation.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 1 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 2 • 11 years ago

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

	Andreas Gal :gal
	Comment 3 • 11 years ago

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.

	Mandyam Vikram
	Comment 4 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 5 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 6 • 11 years ago

bank/jank

	Andreas Gal :gal
	Comment 7 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 8 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 9 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 11 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 12 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 13 • 11 years ago

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.

	Andreas Gal :gal
	Comment 14 • 11 years ago

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.

	Jeff Muizelaar [:jrmuizel]
	Comment 15 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 16 • 11 years ago

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.

	Jeff Muizelaar [:jrmuizel]
	Comment 17 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 18 • 11 years ago

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.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 19 • 11 years ago

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

	Sotaro Ikeda [:sotaro]
	Comment 20 • 11 years ago

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

	Joe Cheng [:jcheng] (please needinfo)
	Comment 21 • 11 years ago

hi Jack, can you help with comment 20? thanks

	Benoit Girard (:BenWa) Assignee
	Comment 22 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 23 • 11 years ago

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

	Jeff Muizelaar [:jrmuizel]
	Comment 24 • 11 years ago

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.

	Andreas Gal :gal
	Comment 25 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 26 • 11 years ago

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.

	Andreas Gal :gal
	Comment 27 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 28 • 11 years ago

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

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 29 • 11 years ago

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

	(no longer active)
	Comment 30 • 11 years ago

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!

	Andreas Gal :gal
	Comment 31 • 11 years ago

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

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 32 • 11 years ago

Thanks - let's consider it for 1.2 then

	Benoit Girard (:BenWa) Assignee
	Comment 33 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 34 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&pusher=b56girard@gmail.com

	Benoit Girard (:BenWa) Assignee
	Comment 35 • 11 years ago

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

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

	Matt Woodrow (:mattwoodrow)
	Comment 36 • 11 years ago

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

	Jack Liu
	Comment 37 • 11 years ago

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

	Jeff Muizelaar [:jrmuizel]
	Comment 38 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 39 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 40 • 11 years ago

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

	Andreas Gal :gal
	Comment 41 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 42 • 11 years ago

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.

	Andreas Gal :gal
	Comment 43 • 11 years ago

Nice.

	Bas Schouten (:bas.schouten)
	Comment 45 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 46 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 47 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 48 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 49 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 50 • 11 years ago

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

	Bas Schouten (:bas.schouten)
	Comment 51 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 52 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 54 • 11 years ago

Fix windows build failure, waiting on CLOSED TREE to push

	Benoit Girard (:BenWa) Assignee
	Comment 55 • 11 years ago

Inbound is still closed, requesting checkin-needed

	Benoit Girard (:BenWa) Assignee
	Comment 56 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 57 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 58 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 59 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 60 • 11 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 61 • 11 years ago

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

	Ed Morley [:emorley]
	Comment 62 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 63 • 11 years ago

Attempt at unbitrotting:

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

	Ryan VanderMeulen [:RyanVM]
	Comment 64 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 65 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 66 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 67 • 11 years ago

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