Closed Bug 929513 Opened 6 years ago Closed 6 years ago

Use gfxSize less in layers

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: dzbarsky, Assigned: torarvid)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(18 files, 14 obsolete files)

43.28 KB, patch
nical
: review+
Details | Diff | Splinter Review
13.76 KB, patch
nical
: review+
kats
: feedback-
Details | Diff | Splinter Review
50.95 KB, patch
nical
: review+
Details | Diff | Splinter Review
3.32 KB, patch
nical
: review+
Details | Diff | Splinter Review
8.89 KB, patch
nical
: review+
Details | Diff | Splinter Review
5.23 KB, patch
nical
: review+
Details | Diff | Splinter Review
6.87 KB, patch
nical
: review+
Details | Diff | Splinter Review
1.71 KB, patch
nical
: review+
Details | Diff | Splinter Review
22.87 KB, patch
nical
: review+
Details | Diff | Splinter Review
9.96 KB, patch
nical
: review+
Details | Diff | Splinter Review
3.59 KB, patch
nical
: review+
Details | Diff | Splinter Review
8.79 KB, patch
nical
: review+
Details | Diff | Splinter Review
5.77 KB, patch
nical
: review+
Details | Diff | Splinter Review
9.65 KB, patch
nical
: review+
Details | Diff | Splinter Review
10.39 KB, patch
nical
: review+
Details | Diff | Splinter Review
57.02 KB, patch
nical
: review+
Details | Diff | Splinter Review
25.86 KB, patch
nical
: review+
Details | Diff | Splinter Review
2.89 KB, patch
nical
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch patchSplinter Review
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #820375 - Flags: review?(nical.bugzilla)
Attachment #820375 - Flags: review?(nical.bugzilla) → review+
Summary: Use gfx::Size more in layers → Use gfxSize less in layers
Attachment #820879 - Flags: review?(nical.bugzilla)
Attached patch PArt 3Splinter Review
Attachment #820941 - Flags: review?(nical.bugzilla)
Attachment #820944 - Flags: review?(nical.bugzilla)
Attachment #820949 - Flags: review?(nical.bugzilla)
Attachment #820879 - Flags: review?(nical.bugzilla) → review+
Attachment #820941 - Flags: review?(nical.bugzilla) → review+
Attachment #820944 - Flags: review?(nical.bugzilla) → review+
Attachment #820949 - Flags: review?(nical.bugzilla) → review+
Attached patch Some cleanupSplinter Review
Attachment #820972 - Flags: review?(nical.bugzilla)
Attachment #820973 - Flags: review?(nical.bugzilla)
Attachment #820979 - Flags: review?(nical.bugzilla)
Attachment #820980 - Flags: review?(nical.bugzilla)
Attachment #820980 - Attachment is obsolete: true
Attachment #820980 - Flags: review?(nical.bugzilla)
Attachment #820972 - Flags: review?(nical.bugzilla) → review+
Attachment #820973 - Flags: review?(nical.bugzilla) → review+
Attachment #820979 - Flags: review?(nical.bugzilla) → review+
Attachment #821657 - Flags: review?(nical.bugzilla)
Attachment #821658 - Flags: review?(nical.bugzilla)
Attachment #821660 - Flags: review?(nical.bugzilla)
Attachment #821661 - Flags: review?(nical.bugzilla)
Attachment #821657 - Flags: review?(nical.bugzilla) → review+
Attachment #821658 - Flags: review?(nical.bugzilla) → review+
Attachment #821660 - Flags: review?(nical.bugzilla) → review+
Attachment #821661 - Flags: review?(nical.bugzilla) → review+
Blocks: 882113
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.
Attachment #820879 - Flags: feedback-
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?
Flags: needinfo?(dzbarsky)
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 :)
Assignee: dzbarsky → nobody
Flags: needinfo?(dzbarsky)
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.
Attachment #8343707 - Flags: review+
Attachment #8343708 - Flags: review+
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.
Attachment #8343710 - Flags: review-
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.
Attachment #8343711 - Flags: review-
(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.
Attachment #8343710 - Attachment is obsolete: true
Attachment #8344538 - Flags: review?(nical.bugzilla)
Attachment #8343711 - Attachment is obsolete: true
Attachment #8344539 - Flags: review?(nical.bugzilla)
This rippled into other replacements in layers and in gfx/gl.
Attachment #8344540 - Flags: review?(nical.bugzilla)
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.
Attachment #8344538 - Flags: review?(nical.bugzilla) → review+
Attachment #8344539 - Flags: review?(nical.bugzilla) → review+
Attachment #8344540 - Flags: review?(nical.bugzilla) → review+
(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 :)
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.
Attachment #8343707 - Attachment is obsolete: true
Attachment #8343708 - Attachment is obsolete: true
Attachment #8345772 - Flags: review?(nical.bugzilla)
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.
Attachment #8344538 - Attachment is obsolete: true
Attachment #8345773 - Flags: review?(nical.bugzilla)
Attachment #8344539 - Attachment is obsolete: true
Attachment #8345774 - Flags: review?(nical.bugzilla)
This rippled into other replacements in layers and in gfx/gl.
Attachment #8344540 - Attachment is obsolete: true
Attachment #8345775 - Flags: review?(nical.bugzilla)
Attachment #8345771 - Flags: review?(nical.bugzilla)
I rebased yesterday, and have attached new patches ready for another go at the try server
Attachment #8345771 - Flags: review?(nical.bugzilla) → review+
Attachment #8345772 - Flags: review?(nical.bugzilla) → review+
Attachment #8345773 - Flags: review?(nical.bugzilla) → review+
Attachment #8345774 - Flags: review?(nical.bugzilla) → review+
Attachment #8345775 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8346501 - Flags: review?(nical.bugzilla)
Btw, :kevsim has already pushed this patch to the try server for me: https://tbpl.mozilla.org/?tree=Try&rev=f69517501a38
Attachment #8346501 - Flags: review?(nical.bugzilla) → review+
(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 :)
Assignee: nobody → torarvid
Attachment #8345771 - Attachment is obsolete: true
Attachment #8347118 - Flags: review?(nical.bugzilla)
Attachment #8345772 - Attachment is obsolete: true
Attachment #8347119 - Flags: review?(nical.bugzilla)
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.
Attachment #8345773 - Attachment is obsolete: true
Attachment #8347120 - Flags: review?(nical.bugzilla)
Attachment #8345774 - Attachment is obsolete: true
Attachment #8347121 - Flags: review?(nical.bugzilla)
This rippled into other replacements in layers and in gfx/gl.
Attachment #8345775 - Attachment is obsolete: true
Attachment #8347122 - Flags: review?(nical.bugzilla)
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.
Attachment #8346501 - Attachment is obsolete: true
Attachment #8347123 - Flags: review?(nical.bugzilla)
nical: No changes in last patch set other than added "r=nical" to the commit msgs
Attachment #8347118 - Flags: review?(nical.bugzilla) → review+
Attachment #8347119 - Flags: review?(nical.bugzilla) → review+
Attachment #8347120 - Flags: review?(nical.bugzilla) → review+
Attachment #8347121 - Flags: review?(nical.bugzilla) → review+
Attachment #8347122 - Flags: review?(nical.bugzilla) → review+
Attachment #8347123 - Flags: review?(nical.bugzilla) → review+
This bug is becoming a bit hard to follow so let's open new bugs for future patches.
Keywords: checkin-needed
Whiteboard: [leave open]
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.
Whiteboard: [qa-]
Depends on: 1006123
You need to log in before you can comment on or make changes to this bug.