Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] ensure 1x1 has mipcount of 1 #39182

Merged
merged 3 commits into from
Jan 27, 2023
Merged

Conversation

jonahwilliams
Copy link
Member

Fixes flutter/flutter#119245

Before mipcount of 1x1 image would be 0, resulting in invalid texture

@jonahwilliams
Copy link
Member Author

actually I think I might be misunderstanding this API, certainly asking for a mipcount of 0 is invalid - but does mipcount of 1 mean no mipping mapping?

@chinmaygarde
Copy link
Member

Yeah, 1 just means no mipmapping.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits but lgtm otherwise.

@@ -102,7 +102,9 @@ struct TSize {
if (!IsPositive()) {
return 1u;
}
return std::max(ceil(log2(width)), ceil(log2(height)));
constexpr size_t minimumMip = 1u;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

underscore_case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -640,6 +640,7 @@ TEST(GeometryTest, CanGenerateMipCounts) {
ASSERT_EQ((Size{128, 0}.MipCount()), 1u);
ASSERT_EQ((Size{128, -25}.MipCount()), 1u);
ASSERT_EQ((Size{-128, 25}.MipCount()), 1u);
ASSERT_EQ((Size{1, 1}.MipCount()), 1u);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mip counts should always at least be 1. Perhaps also check Size{0, 0}?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 27, 2023
@auto-submit auto-submit bot merged commit 6a92731 into flutter:main Jan 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 27, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 27, 2023
…119344)

* 6a92731d2 [Impeller] ensure 1x1 has mipcount of 1 (flutter/engine#39182)

* 55f7a09b4 Roll Fuchsia Linux SDK from kdjOCYzDtnfY30985... to 6c2H32X3EXOGlWIgb... (flutter/engine#39193)

* 621e13cc9 Roll Dart SDK from dcdd3fbb3116 to 2cd9b7ac95e8 (2 revisions) (flutter/engine#39185)
zanderso pushed a commit to zanderso/engine that referenced this pull request Feb 3, 2023
* [Impeller] ensure 1x1 has mipcount of 1

* Update geometry_unittests.cc

* Update size.h
godofredoc pushed a commit that referenced this pull request Feb 4, 2023
* [Impeller] ensure 1x1 has mipcount of 1

* Update geometry_unittests.cc

* Update size.h

Co-authored-by: Jonah Williams <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Impeller] Image.memory(kTransparentImage) crashes the app with Impeller
2 participants