1843524 - Change the type of BaseSize::{width, height} to Coord

Status: ASSIGNED

(Core :: Graphics, task, P3)

Comment 1

[Razvan Cojocaru]

While working on this I've hit something akin to the operator problem discussed here:

 0:27.18 In file included from /mnt/sdcard/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/ImageContainer.h:32:
 0:27.18 /mnt/sdcard/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/MediaInfo.h:392:17: error: use of overloaded operator '
==' is ambiguous (with operand types 'int64_t' (aka 'long') and 'const mozilla::gfx::IntCoordTyped<mozilla::gfx::UnknownUnits>')
 0:27.18     if ((aWidth == mImage.width && aHeight == mImage.height) || !mImage.width ||
 0:27.18          ~~~~~~ ^  ~~~~~~~~~~~~
 0:27.18 /mnt/sdcard/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/BaseCoord.h:83:25: note: candidate function
 0:27.18   friend constexpr bool operator==(T aA, Sub aB) { return aA == aB.value; }
 0:27.18                         ^
 0:27.18 /mnt/sdcard/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/Coord.h:70:15: note: candidate function
 0:27.18   friend bool operator==(Primitive aA, Coord aB) { return aA == aB.value; }
 0:27.18               ^
 0:27.18 /mnt/sdcard/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/Coord.h:70:15: note: candidate function
 0:27.18 /mnt/sdcard/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/BaseCoord.h:37:25: note: candidate function
 0:27.18   friend constexpr bool operator==(Sub aA, Sub aB) {
 0:27.18                         ^
 0:27.18 /mnt/sdcard/work/debug/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/MediaInfo.h:392:17: note: built-in candidate operator==
(long, int)
 0:27.18     if ((aWidth == mImage.width && aHeight == mImage.height) || !mImage.width ||
 0:27.18                 ^

Should we address this before moving on?

Comment 2

[Razvan Cojocaru]

I see great opportunities for reducing the code line count in Coord.h if we can safely assume we're using at least C++14: we can get rid of struct CommonType completely and just return auto:

https://www.modernescpp.com/index.php/automatic-return-type-c-11-14-20

Though that's possibly another independent bug. Let me know what you think.

Comment 3

[Botond Ballo [:botond]]

(In reply to Razvan Cojocaru from comment #2)

I see great opportunities for reducing the code line count in Coord.h if we can safely assume we're using at least C++14: we can get rid of struct CommonType completely and just return auto:

https://www.modernescpp.com/index.php/automatic-return-type-c-11-14-20

Though that's possibly another independent bug. Let me know what you think.

Yes, I think this is a good idea. That code dates back to 2014 when we couldn't use return type deduction yet, but according to https://firefox-source-docs.mozilla.org/code-quality/coding-style/using_cxx_in_firefox_code.html we now can.

Perhaps we can do that refactoring in a dependent bug, and see if it simplifies things here?

Comment 4

[Razvan Cojocaru]

Now that bug 1844406 is done, I've taken another quick look at this, and I got to a point where it appears that we currently do not allow Coord * Coord. Shouldn't we?

 0:05.83 /mnt/sdcard/work/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/2D.h:648:38: error: use of overloaded operator '*' is ambiguous (with operand types 'mozilla::gfx::IntCoordTyped<mozilla::gfx::UnknownUnits>' and 'mozilla::gfx::IntCoordTyped<mozilla::gfx::UnknownUnits>')
 0:05.83     aInfo.mUnknownBytes = size.width * size.height * BytesPerPixel(format);
 0:05.83                           ~~~~~~~~~~ ^ ~~~~~~~~~~~
 0:05.83 /mnt/sdcard/work/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/BaseCoord.h:50:24: note: candidate function
 0:05.83   friend constexpr Sub operator*(Sub aCoord, T aScale) {
 0:05.83                        ^
 0:05.83 /mnt/sdcard/work/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/BaseCoord.h:53:24: note: candidate function
 0:05.83   friend constexpr Sub operator*(T aScale, Sub aCoord) {
 0:05.83                        ^
 0:05.83 /mnt/sdcard/work/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/Coord.h:57:15: note: candidate function
 0:05.83   friend auto operator*(Coord aCoord, Primitive aScale) {
 0:05.83               ^
 0:05.83 /mnt/sdcard/work/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/Coord.h:57:15: note: candidate function
 0:05.84 /mnt/sdcard/work/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/mozilla/gfx/Coord.h:60:15: note: candidate function
 0:05.84   friend auto operator*(Primitive aScale, Coord aCoord) {
 0:05.84               ^

Comment 5

[Razvan Cojocaru]

I can, of course, always make it go away with size.width.value * size.height.value, but just wondering.

Comment 6

[Botond Ballo [:botond]]

Not allowing Coord * Coord is deliberate. Conceptually, the result of such a multiplication has different units - for example, if the inputs are in pixels which denote length, the result would be in "pixels squared" which denotes area.

Note that for the primitive * Coord and Coord * primitive operations that we allow, the primitive passed in usually conceptually a scale -- that is, after being upgraded to typed units, I would expect those to become ScaleFactor * Coord or Coord * ScaleFactor, rather than Coord * Coord.

Comment 7

[Razvan Cojocaru]

(In reply to Botond Ballo [:botond] from comment #6)

Not allowing Coord * Coord is deliberate. Conceptually, the result of such a multiplication has different units - for example, if the inputs are in pixels which denote length, the result would be in "pixels squared" which denotes area.

Note that for the primitive * Coord and Coord * primitive operations that we allow, the primitive passed in usually conceptually a scale -- that is, after being upgraded to typed units, I would expect those to become ScaleFactor * Coord or Coord * ScaleFactor, rather than Coord * Coord.

Makes perfect sense. Thanks!

Comment 8

[Razvan Cojocaru]

Attachment: Bug 1843524 - Change the type of BaseSize::{width, height} to Coord. r?botond,dlrobertson

Comment 9

[Kelsey Gilbert [:jgilbert]]

I would like to know more about the overall plan here.
We use e.g. IntSize for a bunch of {width,height} tuples in Gfx code, and this change sounds like it would make IntSize harder to use for Gfx's usecase.
We (Gfx) can definitely switch to something else if e.g. Layout wants this change though!
(Webgl already uses e.g. uvec2{.x,.y} for this! https://searchfox.org/mozilla-central/source/dom/canvas/WebGLTypes.h#531 )

Comment 10

[Razvan Cojocaru]

(In reply to Kelsey Gilbert [:jgilbert] from comment #9)

I would like to know more about the overall plan here.
We use e.g. IntSize for a bunch of {width,height} tuples in Gfx code, and this change sounds like it would make IntSize harder to use for Gfx's usecase.
We (Gfx) can definitely switch to something else if e.g. Layout wants this change though!
(Webgl already uses e.g. uvec2{.x,.y} for this! https://searchfox.org/mozilla-central/source/dom/canvas/WebGLTypes.h#531 )

The point of this change, as far as I can tell, is to make the code as typesafe as possible. That is, not allow raw (x, y) pairs where you would want to use an object that uses an exact type of Units. The added verbosity where conversions really are needed would then be a feature rather than a problem, because they would pinpoint where these need to happen in the code instead of performing them silently (and potentially inappropriately).

Whether this is worth what has turned out to be the biggest patch I've ever submitted or seen is up to the reviewers.
I'm sure [:botond] will be along soon with more eloquence than I've probably been able to muster. :)

Comment 11

[Botond Ballo [:botond]]

I'm in the process of (slowly) reviewing the patch. In my review, I will share some ideas about how to avoid having to unwrap .value in some situations, though others will definitely remain. I've also been accumulating a list of situations where unwrapping .value can't currently be avoided, which I will share for discussion.

The intent is definitely for unwrapping .value to be the exception rather than the rule, i.e. in most cases we'd like to stay in typed-units land and perform operations on the Coord rather than having to unwrap its raw value.

Regarding this point in particular:

(In reply to Kelsey Gilbert [:jgilbert] from comment #9)

We use e.g. IntSize for a bunch of {width,height} tuples in Gfx code, and this change sounds like it would make IntSize harder to use for Gfx's usecase.
We (Gfx) can definitely switch to something else if e.g. Layout wants this change though!

IntSize is a typedef for IntSizeTyped<UnknownUnits>, and the way we've been thinking about UnknownUnits is "this is a variable that hasn't been refactored to use typed units yet, but will hopefully be in the future", so the hope is that eventually uses of UnknownUnits can be replaced with uses of a concrete unit type (like CSSPixel, ScreenPixel, etc.).

If there are uses of IntSize (and other types using UnknownUnits) where you think they're never likely to be replaced with a concrete unit type, perhaps we could explore using a different type for that (and not change width / height fields of that type to Coord).

I'd say let's wait for the patch to go through an initial round of review and see how it's looking after that (i.e. what is the volume of remaining places where we unwrap .value and how invasive that seems), and then make a decision on a direction going forward.

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)

I've also been accumulating a list of situations where unwrapping .value can't currently be avoided, which I will share for discussion.

Apologies for taking so long to get around to writing this up, but here's the mentioned list of situations that came up during the review of this patch where unwrapping .value can't be avoided with our current abstractions:

• Interfacing with bounds-checking helpers like CheckedInt (example) or AssertedCast (example)
  • Possible solution: write Coord versions of these helpers (e.g. CheckedCoord), or possibly more general versions that can work with various wrapped representations
• Interfacing with APIs that take or return pointers or references to primitive types. There are various examples of this:
  • LogicalPixelSize::ISize and similar
    • Possible solution: add a version of LogicalPixelSize templated on a unit type
  • Resolution::ApplyTo()
    • Possible solution: add appropriate Size overloads
  • Getters generated from IDL bindings, such as imgIContainer::GetWidth() (example) and nsIDocShellTreeOwner::GetPrimaryContentSize (example)
    • Possible solution: where possible, change the IDL signatures so the generated C++ method returns an object rather than assigning to out-parameters. Where that's not currently possible, improve the IDL binding generator to make it possible.
  • Functions at the C++/Rust language boundary generated by cbindgen, such as wr_renderer_record_frame
    • Possible solution: similar to the IDL case, explore improvements to the binding generator to allow these functions to use a return value rather than out-parameters
  • Third-party C APIs. Here, there's not much we can do besides writing wrapper functions around them that localize the mess. Examples:
    • ffmpeg (example)
    • wayland (example)
    • GTK (example)
• Interfacing with APIs that take an unrestricted template parameter (e.g. aVec2::From())
  • (The reason the fact that they take an unrestricted template parameter is relevant is that, if such a function had parameters of a concrete type like uint32_t, Coord arguments would get implicitly converted to the parameter type. But since the parameter type is a template parameter, the template parameter gets deduced as Coord without any conversion.)
  • The best thing I can think of here is to give such functions a Coord overload (which can still be templated on the pixel type) which localize the unwrapping.
• Interfacing with APIs that use C-style format strings like AppendPrintf or MOZ_LOG.
  • This should be improved in the very near future by the adoption of typed format strings in bug 1717448.
• Mixed-type arithmetic operations for which we don't have an unambiguous overload.
  • Some examples that have come up are:
    • unsigned long * IntCoord
    • int64_t == IntCoord
    • IntCoord * uint32_t
    • IntCoord - size_t
    • max(IntCoord, int32_t) (and similarly min)
    • IntCoord - Coord
  • Possible solution: add more mixed-type overloads for arithmetic operations.
• Converting a float Coord to a double Coord
  • Possible solution: add a dedicated Cast() method for this
• Multiplying a width by a ratio of widths
  • Possible solution: introduce an AspectRatio type for representing a ratio of widths or heights
• Various low-level manipulations related to the in-memory representation of data
  • Examples:
    • doing pointer arithmetic by adding size.height to a uintptr_t
    • multiplying size.height by a stride
  • There's probably no one-size-fits-all solution for these.

I think a next step here could be to chip away at some of the improvements identified under "potential solutions" above. Note that these can be implemented independently of each other (and also without needing the WIP patch in this bug). The WIP patch here can then be rebased on top of those, decreasing the amount of unwrapping it needs to do, and we can consider next steps from there.