[rust bmp] Define BMP decoder factory and RustyBMP feature [chromium/src : main]

[Sergio Gonzalez Martin (Gerrit)]

Attention needed from Kaylee Lubick and Łukasz Anforowicz

New activity on the change

Open in Gerrit

Related details

Attention is currently required from:

• Kaylee Lubick
• Łukasz Anforowicz

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I81ccae85235e094dd9b262767672a46891047e59

Gerrit-Change-Number: 7217254

Gerrit-PatchSet: 8

Gerrit-Owner: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Reviewer: Kaylee Lubick <kjlu...@chromium.org>

Gerrit-Reviewer: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Reviewer: Łukasz Anforowicz <luk...@chromium.org>

Gerrit-CC: Anton Yarkov <anton...@microsoft.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Daniel Cheng <dch...@chromium.org>

Gerrit-CC: Florin Malita <fma...@gmail.com>

Gerrit-CC: Gareth Evans <gareth...@microsoft.com>

Gerrit-CC: Johan Rosengren <jrose...@microsoft.com>

Gerrit-CC: Marco Bartoli <marcob...@microsoft.com>

Gerrit-CC: Matthew Riley <mat...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Kaylee Lubick <kjlu...@chromium.org>

Gerrit-Attention: Łukasz Anforowicz <luk...@chromium.org>

Gerrit-Comment-Date: Tue, 23 Dec 2025 22:08:51 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No

[Sergio Gonzalez Martin (Gerrit)]

Attention needed from Kaylee Lubick and Łukasz Anforowicz

Sergio Gonzalez Martin added 1 comment

Commit Message

Line 9, Patchset 8 (Latest):Define a factory class for creating BMP decoder instances
based on RustyBMP feature flag, so if it is enabled it uses
SkBmpRustDecoder, otherwise it keeps using BMPImageDecoder.

Sergio Gonzalez Martin . unresolved

As expected, this change is increasing binary size as we are starting to link the image crate used for Rust BMP decoder. The increase is in line with what we saw in our evaluation during ATL review (it was ~20kB so actual number is even a bit lower)

https://chromium-review.googlesource.com/c/chromium/src/+/7215854

Open in Gerrit

Related details

Attention is currently required from:

• Kaylee Lubick
• Łukasz Anforowicz

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

• No-Unresolved-Comments

• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I81ccae85235e094dd9b262767672a46891047e59

Gerrit-Change-Number: 7217254

Gerrit-PatchSet: 8

Gerrit-Owner: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Reviewer: Kaylee Lubick <kjlu...@chromium.org>

Gerrit-Reviewer: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Reviewer: Łukasz Anforowicz <luk...@chromium.org>

Gerrit-CC: Anton Yarkov <anton...@microsoft.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Daniel Cheng <dch...@chromium.org>

Gerrit-CC: Florin Malita <fma...@gmail.com>

Gerrit-CC: Gareth Evans <gareth...@microsoft.com>

Gerrit-CC: Johan Rosengren <jrose...@microsoft.com>

Gerrit-CC: Marco Bartoli <marcob...@microsoft.com>

Gerrit-CC: Matthew Riley <mat...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Kaylee Lubick <kjlu...@chromium.org>

Gerrit-Attention: Łukasz Anforowicz <luk...@chromium.org>

Gerrit-Comment-Date: Tue, 23 Dec 2025 22:23:18 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Łukasz Anforowicz (Gerrit)]

Attention needed from Kaylee Lubick and Sergio Gonzalez Martin

Łukasz Anforowicz voted and added 5 comments

Votes added by Łukasz Anforowicz

Code-Review

+1

5 comments

Patchset-level comments

File-level comment, Patchset 8 (Latest):

Łukasz Anforowicz . resolved

LGTM with some questions/comments below.

Commit Message

Line 9, Patchset 8 (Latest):Define a factory class for creating BMP decoder instances
based on RustyBMP feature flag, so if it is enabled it uses
SkBmpRustDecoder, otherwise it keeps using BMPImageDecoder.

Sergio Gonzalez Martin . resolved

As expected, this change is increasing binary size as we are starting to link the image crate used for Rust BMP decoder. The increase is in line with what we saw in our evaluation during ATL review (it was ~20kB so actual number is even a bit lower)

https://chromium-review.googlesource.com/c/chromium/src/+/7215854

Łukasz Anforowicz

Ack - nice!

File third_party/blink/renderer/platform/image-decoders/bmp/bmp_image_decoder_test.cc

File third_party/blink/renderer/platform/image-decoders/image_decoder_fuzzer_utils.cc

Open in Gerrit

Related details

Attention is currently required from:

• Kaylee Lubick
• Sergio Gonzalez Martin

Gerrit-Attention: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Comment-Date: Tue, 23 Dec 2025 22:47:08 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Comment-In-Reply-To: Sergio Gonzalez Martin <ser...@microsoft.com>

[Sergio Gonzalez Martin (Gerrit)]

Attention needed from Kaylee Lubick and Łukasz Anforowicz

Sergio Gonzalez Martin added 3 comments

File third_party/blink/renderer/platform/image-decoders/bmp/bmp_image_decoder_test.cc

File third_party/blink/renderer/platform/image-decoders/image_decoder_fuzzer_utils.cc

Open in Gerrit

Related details

Attention is currently required from:

• Kaylee Lubick
• Łukasz Anforowicz

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I81ccae85235e094dd9b262767672a46891047e59

Gerrit-Change-Number: 7217254

Gerrit-PatchSet: 10

Gerrit-Owner: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Reviewer: Kaylee Lubick <kjlu...@chromium.org>

Gerrit-Reviewer: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Reviewer: Łukasz Anforowicz <luk...@chromium.org>

Gerrit-CC: Anton Yarkov <anton...@microsoft.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Daniel Cheng <dch...@chromium.org>

Gerrit-CC: Florin Malita <fma...@gmail.com>

Gerrit-CC: Gareth Evans <gareth...@microsoft.com>

Gerrit-CC: Johan Rosengren <jrose...@microsoft.com>

Gerrit-CC: Marco Bartoli <marcob...@microsoft.com>

Gerrit-CC: Matthew Riley <mat...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Kaylee Lubick <kjlu...@chromium.org>

Gerrit-Attention: Łukasz Anforowicz <luk...@chromium.org>

Gerrit-Comment-Date: Wed, 24 Dec 2025 00:34:37 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Łukasz Anforowicz <luk...@chromium.org>

[Łukasz Anforowicz (Gerrit)]

Attention needed from Kaylee Lubick and Sergio Gonzalez Martin

Łukasz Anforowicz voted and added 2 comments

Votes added by Łukasz Anforowicz

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Łukasz Anforowicz . resolved

LGTM after addressing the feedback related to component build

File third_party/blink/renderer/platform/image-decoders/bmp/bmp_features.h

Line 13, Patchset 10 (Latest):COMPONENT_EXPORT(BMP_FEATURES) BASE_DECLARE_FEATURE(kRustyBmpFeature);

Łukasz Anforowicz . unresolved

Thank you for trying to resolve this - you are really close, but I think that you want to reuse the scaffolding that already exists for this component. Here you probably want to:

• `#include "third_party/blink/renderer/platform/platform_export.h"`
• Use `PLATFORM_EXPORT` instead of `COMPONENT_EXPORT(...)`
• Remove `defines = [ "IS_BMP_FEATURES_IMPL" ]` from `third_party/blink/renderer/platform/image-decoders/BUILD.gn`

Open in Gerrit

Related details

Attention is currently required from:

• Kaylee Lubick
• Sergio Gonzalez Martin

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

• No-Unresolved-Comments

• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I81ccae85235e094dd9b262767672a46891047e59

Gerrit-Change-Number: 7217254

Gerrit-PatchSet: 10

Gerrit-Owner: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Reviewer: Kaylee Lubick <kjlu...@chromium.org>

Gerrit-Reviewer: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Reviewer: Łukasz Anforowicz <luk...@chromium.org>

Gerrit-CC: Anton Yarkov <anton...@microsoft.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Daniel Cheng <dch...@chromium.org>

Gerrit-CC: Florin Malita <fma...@gmail.com>

Gerrit-CC: Gareth Evans <gareth...@microsoft.com>

Gerrit-CC: Johan Rosengren <jrose...@microsoft.com>

Gerrit-CC: Marco Bartoli <marcob...@microsoft.com>

Gerrit-CC: Matthew Riley <mat...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Kaylee Lubick <kjlu...@chromium.org>

Gerrit-Attention: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Comment-Date: Wed, 24 Dec 2025 00:54:46 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Sergio Gonzalez Martin (Gerrit)]

Attention needed from Kaylee Lubick and Łukasz Anforowicz

Sergio Gonzalez Martin added 1 comment

File third_party/blink/renderer/platform/image-decoders/bmp/bmp_features.h

Line 13, Patchset 10:COMPONENT_EXPORT(BMP_FEATURES) BASE_DECLARE_FEATURE(kRustyBmpFeature);

Łukasz Anforowicz . resolved

Thank you for trying to resolve this - you are really close, but I think that you want to reuse the scaffolding that already exists for this component. Here you probably want to:

• `#include "third_party/blink/renderer/platform/platform_export.h"`
• Use `PLATFORM_EXPORT` instead of `COMPONENT_EXPORT(...)`
• Remove `defines = [ "IS_BMP_FEATURES_IMPL" ]` from `third_party/blink/renderer/platform/image-decoders/BUILD.gn`

Sergio Gonzalez Martin

Done

Open in Gerrit

Related details

Attention is currently required from:

• Kaylee Lubick
• Łukasz Anforowicz

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I81ccae85235e094dd9b262767672a46891047e59

Gerrit-Change-Number: 7217254

Gerrit-PatchSet: 11

Gerrit-Owner: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Reviewer: Kaylee Lubick <kjlu...@chromium.org>

Gerrit-Reviewer: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Reviewer: Łukasz Anforowicz <luk...@chromium.org>

Gerrit-CC: Anton Yarkov <anton...@microsoft.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Daniel Cheng <dch...@chromium.org>

Gerrit-CC: Florin Malita <fma...@gmail.com>

Gerrit-CC: Gareth Evans <gareth...@microsoft.com>

Gerrit-CC: Johan Rosengren <jrose...@microsoft.com>

Gerrit-CC: Marco Bartoli <marcob...@microsoft.com>

Gerrit-CC: Matthew Riley <mat...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Kaylee Lubick <kjlu...@chromium.org>

Gerrit-Attention: Łukasz Anforowicz <luk...@chromium.org>

Gerrit-Comment-Date: Wed, 24 Dec 2025 09:55:57 +0000

[Sergio Gonzalez Martin (Gerrit)]

Attention needed from Kaylee Lubick and Łukasz Anforowicz

Sergio Gonzalez Martin added 1 comment

File third_party/blink/renderer/platform/image-decoders/bmp/bmp_features.h

Line 13, Patchset 10:COMPONENT_EXPORT(BMP_FEATURES) BASE_DECLARE_FEATURE(kRustyBmpFeature);

Łukasz Anforowicz . resolved

Thank you for trying to resolve this - you are really close, but I think that you want to reuse the scaffolding that already exists for this component. Here you probably want to:

• `#include "third_party/blink/renderer/platform/platform_export.h"`
• Use `PLATFORM_EXPORT` instead of `COMPONENT_EXPORT(...)`
• Remove `defines = [ "IS_BMP_FEATURES_IMPL" ]` from `third_party/blink/renderer/platform/image-decoders/BUILD.gn`

Sergio Gonzalez Martin

Done

Sergio Gonzalez Martin

It seems that with PLATFORM_EXPORT we don't get it exported for windows debug getting thus a build failure due to linking error:

```
[49/4174] LINK blink_platform_unittests.exe blink_platform_unittests.exe.pdb

..\..\third_party\llvm-build\Release+Asserts\bin\lld-link.exe /OUT:./blink_platform_unittests.exe /nologo -libpath:../../third_party/llvm-build/Release+Asserts/lib/clang/22/lib/windows /winsysroot:../../third_party/depot_tools/win_toolchain/vs_files/e4305f407e /MACHINE:X86 /PDB:./blink_platform_unittests.exe.pdb @./blink_platform_unittests.exe.rsp

lld-link: error: undefined symbol: struct base::Feature const blink::kRustyBmpFeature

>>> referenced by obj/third_party/blink/renderer/platform/image-decoders/unit_tests/bmp_image_decoder_test.obj:(public: __thiscall blink::`anonymous namespace'::BMPImageDecoderTest::BMPImageDecoderTest(void))

>>> referenced by obj/third_party/blink/renderer/platform/image-decoders/unit_tests/bmp_image_decoder_test.obj:(public: __thiscall blink::`anonymous namespace'::BMPImageDecoderTest::BMPImageDecoderTest(void))
```
I think the issue is that both image_decoders component and it's unit tests are built with config `//third_party/blink/renderer/platform:blink_platform_implementation` which causes dll import is not defined in unit test.

Open in Gerrit

Related details

Attention is currently required from:

• Kaylee Lubick
• Łukasz Anforowicz

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I81ccae85235e094dd9b262767672a46891047e59

Gerrit-Change-Number: 7217254

Gerrit-PatchSet: 14

Gerrit-Owner: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Reviewer: Kaylee Lubick <kjlu...@chromium.org>

Gerrit-Reviewer: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Reviewer: Łukasz Anforowicz <luk...@chromium.org>

Gerrit-CC: Anton Yarkov <anton...@microsoft.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Daniel Cheng <dch...@chromium.org>

Gerrit-CC: Florin Malita <fma...@gmail.com>

Gerrit-CC: Gareth Evans <gareth...@microsoft.com>

Gerrit-CC: Johan Rosengren <jrose...@microsoft.com>

Gerrit-CC: Marco Bartoli <marcob...@microsoft.com>

Gerrit-CC: Matthew Riley <mat...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Kaylee Lubick <kjlu...@chromium.org>

Gerrit-Attention: Łukasz Anforowicz <luk...@chromium.org>

Gerrit-Comment-Date: Wed, 24 Dec 2025 11:34:49 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Sergio Gonzalez Martin <ser...@microsoft.com>

Comment-In-Reply-To: Łukasz Anforowicz <luk...@chromium.org>

[Kaylee Lubick (Gerrit)]

Attention needed from Sergio Gonzalez Martin and Łukasz Anforowicz

Kaylee Lubick voted and added 1 comment

Votes added by Kaylee Lubick

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 14 (Latest):

Kaylee Lubick . resolved

skia/BUILD.gn LGTM

Open in Gerrit

Related details

Attention is currently required from:

• Sergio Gonzalez Martin
• Łukasz Anforowicz

Gerrit-Attention: Sergio Gonzalez Martin <ser...@microsoft.com>

Gerrit-Attention: Łukasz Anforowicz <luk...@chromium.org>

Gerrit-Comment-Date: Fri, 26 Dec 2025 13:20:23 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes