Closed Bug 987311 Opened 6 years ago Closed 5 years ago

Convert CompositorTypes.h to typed enums

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

Attachments

(8 files, 7 obsolete files)

2.67 KB, patch
nical
: review+
Details | Diff | Splinter Review
8.27 KB, patch
nical
: review+
Details | Diff | Splinter Review
51.00 KB, patch
nical
: review+
Details | Diff | Splinter Review
27.64 KB, patch
nical
: review+
Details | Diff | Splinter Review
8.09 KB, patch
nical
: review+
Details | Diff | Splinter Review
92.95 KB, patch
nical
: review+
Details | Diff | Splinter Review
20.20 KB, patch
nical
: review+
Details | Diff | Splinter Review
2.26 KB, patch
Details | Diff | Splinter Review
And I mean, convert all of it, including the bitflags such as TextureFlags, now that we have have typed enum bitflags (bug 987290) and serialize them (bug 987305).

This discovered real bugs in ContentClient.cpp mishandling TextureFlags, see details below.
Attached patch 1/6 - TextureFlags core changes (obsolete) — Splinter Review
Attachment #8395861 - Flags: review?(jmuizelaar)
Attachment #8395867 - Flags: review?(jmuizelaar)
Attachment #8395861 - Flags: review?(jmuizelaar) → review+
Attached patch 4/6 - Other enums core changes (obsolete) — Splinter Review
Attachment #8395928 - Flags: review?(jmuizelaar)
Attachment #8395931 - Flags: review?(jmuizelaar)
So here is the big bug in ContentClient.cpp, that part 3 fixes.

In ContentClientRemoteBuffer::BuildTextureClients (and the Deprecated version further down in this file), we have this: (see http://hg.mozilla.org/mozilla-central/file/bb4dd9872236/gfx/layers/client/ContentClient.cpp#l198 ) :

void
ContentClientRemoteBuffer::BuildTextureClients(SurfaceFormat aFormat,
                                               const nsIntRect& aRect,
                                               uint32_t aFlags)
{
  // ... some code ...

  mTextureInfo.mTextureFlags = aFlags & ~TEXTURE_DEALLOCATE_CLIENT;

  // ... some code ...

  if (aFlags & BUFFER_COMPONENT_ALPHA) {
    // ... some code ...
    mTextureInfo.mTextureFlags |= TEXTURE_COMPONENT_ALPHA;
  }

  // ... some code ...
}

Here we see that aFlags is first used as if it were TextureFlags (TEXTURE_DEALLOCATE_CLIENT is a TextureFlags enum value) and later as if it were RotatedContentBuffer flags (BUFFER_COMPONENT_ALPHA is defined in a nested enum in class RotatedContentBuffer).

So both can't be right.

My understanding of this code is that aFlags really is RotatedContentBuffer flags, and the bug is in the first line above, wrongly interpreting it as TextureFlags,

  mTextureInfo.mTextureFlags = aFlags & ~TEXTURE_DEALLOCATE_CLIENT;

while the code further down in this function is actually correctly converting these flags into TextureFlags:

  if (aFlags & BUFFER_COMPONENT_ALPHA) {
    // ... some code ...
    mTextureInfo.mTextureFlags |= TEXTURE_COMPONENT_ALPHA;
  }

The part 3 patch here implements the RotatedContentBuffer --> TextureFlags conversion correctly once and for all, in a static TextureFlagsForRotatedContentBufferFlags function.

The aFlags & ~TEXTURE_DEALLOCATE_CLIENT is interpreted as just the product of earlier confusion and now that we are creating fresh new TextureFlags, we don't need to worry about that.
This is the ContentClient.cpp bug fix on its own, before we do the typed enum conversion.

Hg paleontology traced this to bug 893301, new textures for ContentClient.
Attachment #8397945 - Flags: review?(nical.bugzilla)
This should now be free of behavior changes.
Attachment #8395867 - Attachment is obsolete: true
Attachment #8395867 - Flags: review?(jmuizelaar)
Attachment #8397947 - Flags: review?(jmuizelaar)
Attachment #8397945 - Flags: review?(nical.bugzilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/896ac152e1af
Assignee: nobody → bjacob
Whiteboard: [leave open]
Attachment #8395861 - Attachment is obsolete: true
Attachment #8395863 - Attachment is obsolete: true
Attachment #8395928 - Attachment is obsolete: true
Attachment #8395930 - Attachment is obsolete: true
Attachment #8395931 - Attachment is obsolete: true
Attachment #8397947 - Attachment is obsolete: true
Attachment #8395863 - Flags: review?(jmuizelaar)
Attachment #8395928 - Flags: review?(jmuizelaar)
Attachment #8395930 - Flags: review?(jmuizelaar)
Attachment #8395931 - Flags: review?(jmuizelaar)
Attachment #8397947 - Flags: review?(jmuizelaar)
Attachment #8412570 - Flags: review?(nical.bugzilla)
Attachment #8412572 - Flags: review?(nical.bugzilla)
Attachment #8412574 - Flags: review?(nical.bugzilla)
Attachment #8412575 - Flags: review?(nical.bugzilla)
Attachment #8412570 - Flags: review?(nical.bugzilla) → review+
Attachment #8412572 - Flags: review?(nical.bugzilla) → review+
Attachment #8412573 - Flags: review?(nical.bugzilla) → review+
Attachment #8412574 - Flags: review?(nical.bugzilla) → review+
Attachment #8412575 - Attachment is patch: true
Attachment #8412575 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8412576 [details] [diff] [review]
6/6 - Other enums remaining manual changes

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

::: gfx/layers/CompositorTypes.h
@@ +217,5 @@
>    Front = 1,
>    Back = 2,
>    OnWhiteFront = 3,
> +  OnWhiteBack = 4,
> +  HighBound

Note for later: we should really move the sentinels out the enums. -WError makes them annoying because switch statements now *have* to to contains cases for them (or have an empty default case which completely voids the point of checking that all enum variants are handled).
Attachment #8412576 - Flags: review?(nical.bugzilla) → review+
The problem is we often need the sentinels for various things, such as: validating that enum values are in range when we serialize/deserialize them (cf. ****EnumSerializer); constructing arrays with as many entries as there are enum values (cf. EnumeratedArray).

Having sentinels as enum values is currently the only way to have them automatically have the right value.

In the future, for C++17 there is a proposal to add full compile-time reflection of enums, which would solve that problem.
I got errors with vs2013.2 rc.

C:/develop/mozilla/central/gfx/layers\d3d11/CompositorD3D11.cpp(481) : error C2677: 二进制“[”: 没有找到接受“mozilla::layers::MaskType”类型的全局运算符(或没有可接受的转换)
C:/develop/mozilla/central/gfx/layers\d3d11/CompositorD3D11.cpp(484) : error C2677: 二进制“[”: 没有找到接受“mozilla::layers::MaskType”类型的全局运算符(或没有可接受的转换)
C:/develop/mozilla/central/gfx/layers\d3d11/CompositorD3D11.cpp(488) : error C2677: 二进制“[”: 没有找到接受“mozilla::layers::MaskType”类型的全局运算符(或没有可接受的转换)
C:/develop/mozilla/central/gfx/layers\d3d11/CompositorD3D11.cpp(489) : error C2677: 二进制“[”: 没有找到接受“mozilla::layers::MaskType”类型的全局运算符(或没有可接受的转换)
......
Getting other reports like that in comment 21 in #developers, so this was backed out (along with bug 989144 and bug 989027, which depended on it).

http://hg.mozilla.org/mozilla-central/rev/0ecc4193e630
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
After recent pull, with no patches in queue on Win8.1 Update 1 w/ VS2012 I was crashing due to changeset: 64bd above: https://pastebin.mozilla.org/4972777

Swapped to Ubuntu and crashed trying to build Android/Fennec: https://pastebin.mozilla.org/4973038
By "crashing" you mean "failed to compile", right?  I seem to see only build errors in these pastebins.

Thanks for the reports, looking into it, will try to reland asap (this bitrots super fast).

Can I build with MSVC 2012 on tryserver?

Regarding your Fennec build error, what compiler/SDK/NDK are you using?
Target Milestone: mozilla31 → ---
Sorry for the imprecise terminology ... yes, build was failing.

When building for Fennec, my mozconfig is: https://pastebin.mozilla.org/4980730
Not requiring review, as it's basically a trivially required consequence of the other changes here. Arrays indexed by typed enums should be EnumeratedArray's, so that the strong typing is consistent.

Still needinfo'ing Bas mostly in a "FYI" fashion. Also, I hope that I got it right style-wise.
Flags: needinfo?(bas)
Thanks! :)
Flags: needinfo?(bas)
So this fixes the build on MSVC 2012; the last reported issue was that DIAGNOSTIC_NONE error on Android, but this looks like a posisble mis-merge, as it built fine on Android slaves and there is no occurence left of DIAGNOSTIC_NONE after my patches. I'm going to try relanding now.
You need to log in before you can comment on or make changes to this bug.