[//ui] Introduce DisplayColorSpaces constructor taking in SIFormat [chromium/src : main]
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Dave TapuskaHi Dave,
Would you be able to advise on the best way to mitigate the `analyze` failure on the ios-simulator bot here? It's not obvious to me, and I don't have an iOS build to do any iteration with locally. Thanks!
Colin BlundellIt looks to me that causing components/viz/common causes components/viz/test:test_support to be hauled in that causes //services/viz to be hauled in and then that hauls in webnn
Colin BlundellHi Dave,
Thanks! That makes sense to me, but it's not obvious to me what the best path on eliminating this problem is. The dependency I'm adding here is pretty vanilla. I tried making the //components/viz/service dependency from //components/viz/test:test_support conditional on use_blink, but that just moves the problem to somewhere else. Without an iOS checkout, this will be painful to iterate on.
+Sylvain, would you potentially be able to advise on a path to eliminating the problem that this CL is triggering on the iOS non-Blink build? Thanks!
Trying out moving the GN target in question (`shared_image_format`) to its own GN file as a workaround, since its files are in a subdir anyway. If this works I'll split that out to a precursor CL and then it's no longer important to resolve the question about the iOS dependency issue.
Related details
- Dave Tapuska
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Dave TapuskaHi Dave,
Would you be able to advise on the best way to mitigate the `analyze` failure on the ios-simulator bot here? It's not obvious to me, and I don't have an iOS build to do any iteration with locally. Thanks!
Colin BlundellIt looks to me that causing components/viz/common causes components/viz/test:test_support to be hauled in that causes //services/viz to be hauled in and then that hauls in webnn
Colin BlundellHi Dave,
Thanks! That makes sense to me, but it's not obvious to me what the best path on eliminating this problem is. The dependency I'm adding here is pretty vanilla. I tried making the //components/viz/service dependency from //components/viz/test:test_support conditional on use_blink, but that just moves the problem to somewhere else. Without an iOS checkout, this will be painful to iterate on.
+Sylvain, would you potentially be able to advise on a path to eliminating the problem that this CL is triggering on the iOS non-Blink build? Thanks!
Trying out moving the GN target in question (`shared_image_format`) to its own GN file as a workaround, since its files are in a subdir anyway. If this works I'll split that out to a precursor CL and then it's no longer important to resolve the question about the iOS dependency issue.
OK, I'm unfortunately a little stuck here. The `shared_image_format` itself has problematic deps (see latest PS), but the target that I'm adding the dep to is needed on iOS non-Blink (see [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/6988211?tab=checks)).
Dave, is there someone on your side who would be able to take a look at this with me to determine a solution? We need to be able to add the dep here as we're in the process of replacing BufferFormat with SharedImageFormat globally in the codebase.
Related details
- Dave Tapuska
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne added 1 comment![]( "Open in Gerrit")
"//components/viz/common/resources:shared_image_format",This is the line that cause the ios build failure.
`//ui/gfx` is used on iOS but `//components/viz` is currently not, and cannot be used.
So this needs to be conditional.
```
if (!is_apple || use_blink) {
deps += [ "//components/viz/common/resources:shared_image_format" ]
}
```
If I comment this line `gn gen` pass successfully on iOS.
Related details
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne added 4 comments![]( "Open in Gerrit")
DisplayColorSpaces(const ColorSpace& color_space,ditto
#include "components/viz/common/resources/shared_image_format.h"This file is currently built on iOS, so you cannot use this dependency unconditionally. This needs to be behind an
```
#if !BUILDFLAG(IS_IOS) || BUILDFLAG(USE_BLINK)
...
#endif
```
#include "components/viz/common/resources/shared_image_format_utils.h"ditto
DisplayColorSpaces::DisplayColorSpaces(const ColorSpace& c,ditto
Related details
- Colin Blundell
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne added 1 comment![]( "Open in Gerrit")
The following patch fixes the compilation for iOS:
```
diff --git i/ui/gfx/BUILD.gn w/ui/gfx/BUILD.gn
index 44e008037cd27..9f51ef1007fa9 100644
--- i/ui/gfx/BUILD.gn
+++ w/ui/gfx/BUILD.gn
@@ -477,7 +477,6 @@ component("color_space") {
]
}
deps = [
- "//components/viz/common/resources:shared_image_format",
"//skia:skcms",
"//ui/gfx:buffer_types",
"//ui/gfx:gfx_switches",
@@ -485,9 +484,14 @@ component("color_space") {
]
public_deps = [
"//base",
+ "//build:blink_buildflags",
"//skia",
]
+ if (!is_ios || use_blink) {
+ deps += [ "//components/viz/common/resources:shared_image_format" ]
+ }
+
if (is_apple && use_blink) {
sources += [
"hdr_metadata_mac.h",
diff --git i/ui/gfx/display_color_spaces.cc w/ui/gfx/display_color_spaces.cc
index d86b673c92e24..e183d8d3ef2cb 100644
--- i/ui/gfx/display_color_spaces.cc
+++ w/ui/gfx/display_color_spaces.cc
@@ -12,10 +12,14 @@
#include <array>
#include <cmath>
+#include "build/blink_buildflags.h"
#include "build/build_config.h"
-#include "components/viz/common/resources/shared_image_format_utils.h"
#include "skia/ext/skcolorspace_primaries.h"
+#if !BUILDFLAG(IS_IOS) || BUILDFLAG(USE_BLINK)
+#include "components/viz/common/resources/shared_image_format_utils.h" // nogncheck
+#endif
+
namespace gfx {
namespace {
@@ -82,10 +86,12 @@ DisplayColorSpaces::DisplayColorSpaces(const ColorSpace& c, BufferFormat f)
}
}
+#if !BUILDFLAG(IS_IOS) || BUILDFLAG(USE_BLINK)
DisplayColorSpaces::DisplayColorSpaces(const ColorSpace& c,
viz::SharedImageFormat f)
: DisplayColorSpaces(c,
viz::SinglePlaneSharedImageFormatToBufferFormat(f)) {}
+#endif
void DisplayColorSpaces::SetOutputBufferFormats(
gfx::BufferFormat buffer_format_no_alpha,
diff --git i/ui/gfx/display_color_spaces.h w/ui/gfx/display_color_spaces.h
index 3232b0cfde0f6..dfe1e8015cd56 100644
--- i/ui/gfx/display_color_spaces.h
+++ w/ui/gfx/display_color_spaces.h
@@ -10,13 +10,18 @@
#include <vector>
#include "base/memory/ref_counted.h"
-#include "components/viz/common/resources/shared_image_format.h"
+#include "build/blink_buildflags.h"
+#include "build/build_config.h"
#include "skia/ext/skcolorspace_primaries.h"
#include "third_party/skia/include/core/SkColorSpace.h"
#include "ui/gfx/buffer_types.h"
#include "ui/gfx/color_space.h"
#include "ui/gfx/color_space_export.h"
+#if !BUILDFLAG(IS_IOS) || BUILDFLAG(USE_BLINK)
+#include "components/viz/common/resources/shared_image_format.h" // nogncheck
+#endif
+
namespace mojo {
template <class T, class U>
struct StructTraits;
@@ -62,11 +67,13 @@ class COLOR_SPACE_EXPORT DisplayColorSpaces {
// sRGB.
DisplayColorSpaces(const ColorSpace& color_space, BufferFormat buffer_format);
+#if !BUILDFLAG(IS_IOS) || BUILDFLAG(USE_BLINK)
// Initialize as |color_space| and |format| (which must be single-plane) for
// all settings. If |color_space| is the default (invalid) color space, then
// initialize to sRGB.
DisplayColorSpaces(const ColorSpace& color_space,
viz::SharedImageFormat format);
+#endif
// Set the color space and buffer format for the final output surface when the
// specified content is being displayed.
```
Sylvain Defresne (Gerrit)
Sylvain Defresne added 2 comments![]( "Open in Gerrit")
Uploaded the original CL with this patch as https://chromium-review.googlesource.com/c/chromium/src/+/6989210 to confirm this fixes all issues with iOS.
Re-upl
Colin Blundell (Gerrit)
Colin Blundell voted and added 3 comments![]( "Open in Gerrit")
Votes added by Colin Blundell
| Commit-Queue | +1 |
3 comments
DisplayColorSpaces(const ColorSpace& color_space,ditto
Hmm, but the point here is to get rid of the flows taking in `BufferFormat` over time - introducing usage of `SharedImageFormat` via new flows is just to allow incremental conversion. Am I understanding correctly that `DisplayColorSpaces` is *used* on iOS without Blink, or is it just *built* on iOS without Blink (you might not know offhand of course)?
#include "components/viz/common/resources/shared_image_format.h"This file is currently built on iOS, so you cannot use this dependency unconditionally. This needs to be behind an
```
#if !BUILDFLAG(IS_IOS) || BUILDFLAG(USE_BLINK)
...
#endif
```
- Colin Blundell
- Dave Tapuska
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne added 1 comment![]( "Open in Gerrit")
DisplayColorSpaces(const ColorSpace& color_space,Colin Blundellditto
Hmm, but the point here is to get rid of the flows taking in `BufferFormat` over time - introducing usage of `SharedImageFormat` via new flows is just to allow incremental conversion. Am I understanding correctly that `DisplayColorSpaces` is *used* on iOS without Blink, or is it just *built* on iOS without Blink (you might not know offhand of course)?
It is built and the linker does not consider this code as dead.
```
$ nm out/Debug-iphonesimulator/Chromium.app/Chromium|c++filt|grep gfx::DisplayColorSpaces|wc -l
71
```
Trying to remove the file from the build on iOS leads to linker failures, because the class `DisplayColorSpaces` is used, amongst other by `display::Display` (the linker only reports the first error).
There is a direct dependency of `//ios/chrome/browser/web/model` on `//ui/display` (which contains `display::Display`) where the iOS code uses `display::ScopedNativeScreen` which returns instances of `display::Display`.
So this code is really used on iOS.
Related details
- Colin Blundell
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 2 comments![]( "Open in Gerrit")
Sylvain, on reflection I still have a question for you. If you're able to provide any insight on it that would be great. Thanks!
"//components/viz/common/resources:shared_image_format",This is the line that cause the ios build failure.
`//ui/gfx` is used on iOS but `//components/viz` is currently not, and cannot be used.
So this needs to be conditional.
```
if (!is_apple || use_blink) {
deps += [ "//components/viz/common/resources:shared_image_format" ]
}
```If I comment this line `gn gen` pass successfully on iOS.
On reflection, I'm still confused about this. The `analyze` error on this PS is the following:
ERROR at //services/webnn/features.gni:8:1: Assertion failed.
assert(use_blink)
^-----
See //services/webnn/BUILD.gn:8:1: whence it was imported.
import("//services/webnn/features.gni")
^-------------------------------------
See //components/viz/service/BUILD.gn:316:5: which caused the file to be included.
"//services/webnn:webnn_service",
I can't see how adding the dep here results in `//components/viz/service/BUILD.gn` being pulled in. Is there something obvious I'm missing/some way that I could analyze this question locally via GN? It's definitely not a dep on `//components/viz/service` itself, as that dep goes the other way (i.e., `//components/viz/service` depends on `//ui/gfx:color_space`).
Related details
- Dave Tapuska
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
"//components/viz/common/resources:shared_image_format",Colin BlundellThis is the line that cause the ios build failure.
`//ui/gfx` is used on iOS but `//components/viz` is currently not, and cannot be used.
So this needs to be conditional.
```
if (!is_apple || use_blink) {
deps += [ "//components/viz/common/resources:shared_image_format" ]
}
```If I comment this line `gn gen` pass successfully on iOS.
On reflection, I'm still confused about this. The `analyze` error on this PS is the following:
ERROR at //services/webnn/features.gni:8:1: Assertion failed.
assert(use_blink)
^-----
See //services/webnn/BUILD.gn:8:1: whence it was imported.
import("//services/webnn/features.gni")
^-------------------------------------
See //components/viz/service/BUILD.gn:316:5: which caused the file to be included.
"//services/webnn:webnn_service",
I can't see how adding the dep here results in `//components/viz/service/BUILD.gn` being pulled in. Is there something obvious I'm missing/some way that I could analyze this question locally via GN? It's definitely not a dep on `//components/viz/service` itself, as that dep goes the other way (i.e., `//components/viz/service` depends on `//ui/gfx:color_space`).
To be clear, "this PS" above refers to PS4, where I pulled out `SharedImageFormat` into a standalone BUILD.gn file.
Sylvain Defresne (Gerrit)
Sylvain Defresne added 1 comment![]( "Open in Gerrit")
"//components/viz/common/resources:shared_image_format",Colin BlundellThis is the line that cause the ios build failure.
`//ui/gfx` is used on iOS but `//components/viz` is currently not, and cannot be used.
So this needs to be conditional.
```
if (!is_apple || use_blink) {
deps += [ "//components/viz/common/resources:shared_image_format" ]
}
```If I comment this line `gn gen` pass successfully on iOS.
Colin BlundellOn reflection, I'm still confused about this. The `analyze` error on this PS is the following:
ERROR at //services/webnn/features.gni:8:1: Assertion failed.
assert(use_blink)
^-----
See //services/webnn/BUILD.gn:8:1: whence it was imported.
import("//services/webnn/features.gni")
^-------------------------------------
See //components/viz/service/BUILD.gn:316:5: which caused the file to be included.
"//services/webnn:webnn_service",
I can't see how adding the dep here results in `//components/viz/service/BUILD.gn` being pulled in. Is there something obvious I'm missing/some way that I could analyze this question locally via GN? It's definitely not a dep on `//components/viz/service` itself, as that dep goes the other way (i.e., `//components/viz/service` depends on `//ui/gfx:color_space`).
To be clear, "this PS" above refers to PS4, where I pulled out `SharedImageFormat` into a standalone BUILD.gn file.
The reason this is loaded:
```
Loading //components/viz/common/resources/BUILD.gn (referenced from //ui/gfx/BUILD.gn:480)
Loading //services/viz/public/mojom/BUILD.gn (referenced from //components/viz/common/resources/BUILD.gn:22)
Loading //components/viz/common/BUILD.gn (referenced from //services/viz/public/mojom/BUILD.gn:94)
Loading //components/viz/test/BUILD.gn (referenced from //components/viz/common/BUILD.gn:326)
Loading //components/viz/service/BUILD.gn (referenced from //components/viz/test/BUILD.gn:104)
```
If `//path1/foo:foo` depends on `//path2/bar:bar`, then it will load `//path2/bar/BUILD.gn` and then process all targets defined in that file and try to load them, even if there is no path between them.
So if `//path2/bar/BUILD.gn` also defines `//path2/bar:must_not_be_used_on_ios` then it will be loaded and cause this failure.
Related details
- Colin Blundell
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne added 1 comment![]( "Open in Gerrit")
"//components/viz/common/resources:shared_image_format",If you want to split, then you also need to split this target.
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Awesome, thanks Sylvain! I think this should give me what I need to go on. Will loop you back in if I have any further questions.
Related details
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 5 comments![]( "Open in Gerrit")
"//components/viz/common/resources:shared_image_format",Thank you!
DisplayColorSpaces(const ColorSpace& color_space,Colin Blundellditto
Sylvain DefresneHmm, but the point here is to get rid of the flows taking in `BufferFormat` over time - introducing usage of `SharedImageFormat` via new flows is just to allow incremental conversion. Am I understanding correctly that `DisplayColorSpaces` is *used* on iOS without Blink, or is it just *built* on iOS without Blink (you might not know offhand of course)?
It is built and the linker does not consider this code as dead.
```
$ nm out/Debug-iphonesimulator/Chromium.app/Chromium|c++filt|grep gfx::DisplayColorSpaces|wc -l
71
```Trying to remove the file from the build on iOS leads to linker failures, because the class `DisplayColorSpaces` is used, amongst other by `display::Display` (the linker only reports the first error).
There is a direct dependency of `//ios/chrome/browser/web/model` on `//ui/display` (which contains `display::Display`) where the iOS code uses `display::ScopedNativeScreen` which returns instances of `display::Display`.
So this code is really used on iOS.
Acknowledged
#include "components/viz/common/resources/shared_image_format.h"Colin BlundellThis file is currently built on iOS, so you cannot use this dependency unconditionally. This needs to be behind an
```
#if !BUILDFLAG(IS_IOS) || BUILDFLAG(USE_BLINK)
...
#endif
```
Thanks! See comment below.
Acknowledged
#include "components/viz/common/resources/shared_image_format_utils.h"Colin Blundellditto
Acknowledged
DisplayColorSpaces::DisplayColorSpaces(const ColorSpace& c,Colin Blundellditto
Acknowledged
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 2 comments![]( "Open in Gerrit")
Would you be able to advise on the best way to mitigate the `analyze` failure on the ios-simulator bot here? It's not obvious to me, and I don't have an iOS build to do any iteration with locally. Thanks!
Colin BlundellIt looks to me that causing components/viz/common causes components/viz/test:test_support to be hauled in that causes //services/viz to be hauled in and then that hauls in webnn
Colin BlundellHi Dave,
Thanks! That makes sense to me, but it's not obvious to me what the best path on eliminating this problem is. The dependency I'm adding here is pretty vanilla. I tried making the //components/viz/service dependency from //components/viz/test:test_support conditional on use_blink, but that just moves the problem to somewhere else. Without an iOS checkout, this will be painful to iterate on.
+Sylvain, would you potentially be able to advise on a path to eliminating the problem that this CL is triggering on the iOS non-Blink build? Thanks!
Colin BlundellTrying out moving the GN target in question (`shared_image_format`) to its own GN file as a workaround, since its files are in a subdir anyway. If this works I'll split that out to a precursor CL and then it's no longer important to resolve the question about the iOS dependency issue.
OK, I'm unfortunately a little stuck here. The `shared_image_format` itself has problematic deps (see latest PS), but the target that I'm adding the dep to is needed on iOS non-Blink (see [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/6988211?tab=checks)).
Dave, is there someone on your side who would be able to take a look at this with me to determine a solution? We need to be able to add the dep here as we're in the process of replacing BufferFormat with SharedImageFormat globally in the codebase.
Done
"//components/viz/common/resources:shared_image_format",Colin BlundellIf you want to split, then you also need to split this target.
Acknowledged
Related details
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vasiliy Telezhnikov
| Code-Review | +1 |
1 comment
Related details
- Colin Blundell
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
10 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[//ui] Introduce DisplayColorSpaces constructor taking in SIFormat
This CL kicks off transitioning DisplayColorSpaces to use
SharedImageFormat rather than BufferFormat.
It introduces a constructor that will allow incrementally converting
callers of DisplayColorSpaces(ColorSpace, BufferFormat) to pass
SharedImageFormat instead and eliminating the former.
I verified that all users of the existing method pass a single-plane
BufferFormat, but in any case, this will become self-evident in the
CLs actually doing the changes.
- M ui/gfx/BUILD.gn
- M ui/gfx/DEPS
- M ui/gfx/display_color_spaces.cc
- M ui/gfx/display_color_spaces.h
Code-Review: +1 by Vasiliy Telezhnikov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |