Last one seems to be failing try. Pushed the others https://hg.mozilla.org/integration/mozilla-inbound/rev/9b86b4e60b29 https://hg.mozilla.org/integration/mozilla-inbound/rev/6efcb3826227 https://hg.mozilla.org/integration/mozilla-inbound/rev/02bc8f1178a6 https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbcde0e9bbd https://hg.mozilla.org/integration/mozilla-inbound/rev/51130b50a7df https://hg.mozilla.org/integration/mozilla-inbound/rev/381baaf48bd2 https://hg.mozilla.org/integration/mozilla-inbound/rev/0f15e95da259 https://hg.mozilla.org/integration/mozilla-inbound/rev/491ba58e1d90 https://hg.mozilla.org/integration/mozilla-inbound/rev/4b587a48c792 https://hg.mozilla.org/integration/mozilla-inbound/rev/35a4cfc41965 https://hg.mozilla.org/integration/mozilla-inbound/rev/f872d288480b

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a2eecdb6d6b0 for failing to build on Windows.

Comment on attachment 820879 [details] [diff] [review] More conversions, using LayerSize Review of attachment 820879 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/CompositableHost.h @@ +41,5 @@ > { > nsIntRegion mVisibleRegion; > nsIntRegion mValidRegion; > gfxRect mDisplayPort; > + LayerSize mEffectiveResolution; This is not really a size. gfxSize was being abused before to store a two-axis resolution, but you shouldn't abuse LayerSize the same way, because LayerSize implies (and enforces at compile-time) that it is actually a size in Layer pixels, which this is not.

A glance through some of the other patches also shows that LayerSize and LayerIntSize are being used for things that are not actually sizes in Layer pixels. Please don't do this.

Hi. Is anyone still working on this? Otherwise, would this be a good First Bug for me to work on? From the existing patches and reviews, it seems that they are mostly OK - except for these LayerSize/LayerIntSize things that should be changed in certain areas. In addition, it seems that there is an issue with building for Windows. (Where can I see the details of the Windows build failures?) Am I correct in assuming that these two things are the remaining TODOs for this bug?

David, still planning to land these patches?

Tor, in the mean time you can look at http://www.joshmatthews.net/bugsahoy/?gfx=1&layout=1&cpp=1 which shows shows a list of mentored bugs and see if there's something you would like to work on

I will speak for dzbarsky here and he is happy to give this bug to Tor :)

I won't have time to finish this up for a while. Tor, feel free to take this :)

Ok, thanks guys. I'm mostly offline until monday. Will look at it then. (I'm part of the Telenor team looking to help Firefox/ffos, btw)

The purpose of this patch is to replace usages of gfxIntSize with gfx::IntSize in BasicCompositor. The new class YUVUtils has two methods ported from gfxUtils in thebes. BasicCompositor now uses these methods instead of the ones in gfxUtils. Also changed gfxIntSize to gfx::IntSize and gfxImageFormat to gfx::SurfaceFormat.

I have now attached 4 patches. There is more work to be done, but these patches are self-contained. Since this is my first mozilla contribution, I thought that it would be nice to get some reviews before I had 10++ patches ready.

Comment on attachment 8343710 [details] [diff] [review] Add YUVUtils class and change gfxIntSize in BasicCompositor Review of attachment 8343710 [details] [diff] [review]: ----------------------------------------------------------------- I think YUVUtils should not be in the layers/ directory, gfx/ycbcr is a better place. Also let's call the file YCbCrUtils instead (we have a history of inconsistently mixing "YUV" and "YCbCr" but we should use the term "YCbCr") ::: gfx/layers/YUVUtils.h @@ +14,5 @@ > +namespace gfx { > + > +class YUVUtils { > +public: > + static void GetYCbCrToRGBDestFormatAndSize(const layers::PlanarYCbCrData& aData, Please use plain functions instead of static methods if there will be no instance and the class has no state. I know that gfxUtils is doing that too but I'd rather not add more of this pattern.

Comment on attachment 8343711 [details] [diff] [review] Replace some instances of gfxIntSize with gfx::IntSize Review of attachment 8343711 [details] [diff] [review]: ----------------------------------------------------------------- Please go through the places where you included 2D.h and include mozilla/gfx/Points.h instead when you just need gfx::IntSize The rest looks good.

(In reply to Tor Arvid Lund [:torarvid] from comment #28) > I have now attached 4 patches. There is more work to be done, but these > patches are self-contained. Since this is my first mozilla contribution, I > thought that it would be nice to get some reviews before I had 10++ patches > ready. You should ask people for review specifically using the r? field when uploading the patches. Otherwise the risk is that people will not notice that there are patches ready for review (or some may notice it, but are busy enough to wait for someone else to do it and then everybody forgets). Once the r? flag is set to someone it's harder to miss it because it shows up in the person's bugzilla dashboard. If you don't know who to ask for review you can look at the names that show up a lot in the hg blame around the code that you are modifying.

The purpose of this patch is to replace usages of gfxIntSize with gfx::IntSize in BasicCompositor. The new class YCbCrUtils has two methods ported from gfxUtils in thebes. BasicCompositor now uses these methods instead of the ones in gfxUtils. Also changed gfxIntSize to gfx::IntSize and gfxImageFormat to gfx::SurfaceFormat.

This rippled into other replacements in layers and in gfx/gl.

Comment on attachment 8344538 [details] [diff] [review] Add YCbCrUtils class and change gfxIntSize in BasicCompositor Review of attachment 8344538 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/ycbcr/YCbCrUtils.h @@ +6,5 @@ > +#ifndef MOZILLA_GFX_UTILS_H_ > +#define MOZILLA_GFX_UTILS_H_ > + > +#include "mozilla/gfx/Types.h" > +#include "ImageContainer.h" nit: Please use forward declarations in header files whenever possible. If you don't mind, I'll fix it locally just before pushing to try and landing so that we don't need another review cycle for a very small change like this.

(In reply to Nicolas Silva [:nical] from comment #35) > nit: Please use forward declarations in header files whenever possible. If > you don't mind, I'll fix it locally just before pushing to try and landing > so that we don't need another review cycle for a very small change like this. Sure, feel free! I'll try and remember that next time :)

try push (last 5 patches) https://tbpl.mozilla.org/?tree=Try&rev=d07f4f34d197

Ok, I see from the try server that I have compile errors on Windows and B2G. Easy to fix, but I'll need to setup some other environments to reproduce it. Will fix tomorrow.

The purpose of this patch is to replace usages of gfxIntSize with gfx::IntSize in BasicCompositor. The new class YCbCrUtils has two methods ported from gfxUtils in thebes. BasicCompositor now uses these methods instead of the ones in gfxUtils. Also changed gfxIntSize to gfx::IntSize and gfxImageFormat to gfx::SurfaceFormat.

This rippled into other replacements in layers and in gfx/gl.

I rebased yesterday, and have attached new patches ready for another go at the try server

try push https://tbpl.mozilla.org/?tree=Try&rev=10c6162bb3ad

Forgot an include in the ImageLayerD3D9.cpp file (though I am unable to reproduce the issue on my own windows installation). Also seems in my previous attempt to fix the build, I introduced a breakage for Android. Hopefully this will fix things.

Btw, :kevsim has already pushed this patch to the try server for me: https://tbpl.mozilla.org/?tree=Try&rev=f69517501a38

(In reply to Tor Arvid Lund [:torarvid] from comment #46) > Forgot an include in the ImageLayerD3D9.cpp file (though I am unable to > reproduce the issue on my own windows installation). Now that we use unified builds, the build system concatenate batches .cpp files before building them, which means that sometimes we forget an include but it doesn't break. Then as the list of concatenated files is not set in stone, the missing #include error can appear with a different build configuration or after someone added/removed .cpp files.

(In reply to Nicolas Silva [:nical] from comment #48) > Now that we use unified builds, the build system concatenate batches .cpp > files before building them, which means that sometimes we forget an include > but it doesn't break. Then as the list of concatenated files is not set in > stone, the missing #include error can appear with a different build > configuration or after someone added/removed .cpp files. Riiiight :) I have noticed that before. At that point, I set --disable-unified-compilation on my Mac - but I never did on Windows... That's probably the reason, then. Thanks. This bug is a bit hard to test thoroughly on my local machine, so sorry for the many iterations.. It's easy to see that "when i touch <whatever>D3D9 files, I need to do a windows build", but not always easy to see when I change something that will only be compiled for android/linux/B2G... (And then I might make mistakes that I don't detect until on the try server). Well, finger crossed that I'll get better with time :)

(In reply to Tor Arvid Lund [:torarvid] from comment #49) > This bug is a bit hard to test thoroughly on my local machine, so sorry for > the many iterations.. It's easy to see that "when i touch <whatever>D3D9 > files, I need to do a windows build", but not always easy to see when I > change something that will only be compiled for android/linux/B2G... (And > then I might make mistakes that I don't detect until on the try server). Don't worry, that's why we have we have try servers. There isn't much you can do to facilitate this kind of task except turning your own desk into a build farm :)

The purpose of this patch is to replace usages of gfxIntSize with gfx::IntSize in BasicCompositor. The new class YCbCrUtils has two methods ported from gfxUtils in thebes. BasicCompositor now uses these methods instead of the ones in gfxUtils. Also changed gfxIntSize to gfx::IntSize and gfxImageFormat to gfx::SurfaceFormat.

This rippled into other replacements in layers and in gfx/gl.

Forgot an include in the ImageLayerD3D9.cpp file (though I am unable to reproduce the issue on my own windows installation). Also seems in my previous attempt to fix the build, I introduced a breakage for Android. Hopefully this will fix things.

nical: No changes in last patch set other than added "r=nical" to the commit msgs

try push https://tbpl.mozilla.org/?tree=Try&rev=7f1e944acc16

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ef7a2f35d7b https://hg.mozilla.org/integration/mozilla-inbound/rev/526236089c17 https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ec1ee06e9d https://hg.mozilla.org/integration/mozilla-inbound/rev/c4cb33486816 https://hg.mozilla.org/integration/mozilla-inbound/rev/ad2dde0902fd https://hg.mozilla.org/integration/mozilla-inbound/rev/a39d288480a8

This bug is becoming a bit hard to follow so let's open new bugs for future patches.

https://hg.mozilla.org/mozilla-central/rev/8ef7a2f35d7b https://hg.mozilla.org/mozilla-central/rev/526236089c17 https://hg.mozilla.org/mozilla-central/rev/a4ec1ee06e9d https://hg.mozilla.org/mozilla-central/rev/c4cb33486816 https://hg.mozilla.org/mozilla-central/rev/ad2dde0902fd https://hg.mozilla.org/mozilla-central/rev/a39d288480a8

FYI - Either this bug or bug 893304 has just caused a large array of gfx layers crashes in bug 950589 that are blocking multiple smoketests.

As I've seen from the comments on 950589, Morris confirmed that 893304 was the cause of the crashes. Please comment again if I should investigate on my end.