-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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] Guard against empty grid sizes #40769
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides using the validation logs, lgtm.
@@ -55,6 +55,8 @@ | |||
return false; | |||
} | |||
|
|||
FML_DCHECK(!grid_size_.IsEmpty() && !thread_group_size_.IsEmpty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use VALIDATION_LOG for such things and return false from here maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's too strong. VALIDATION_LOG will kill the process. An application could get here by mistake and should fizzle instead of dying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(but a test should die)
@@ -47,6 +47,11 @@ bool ComputePass::AddCommand(ComputeCommand command) { | |||
} | |||
|
|||
bool ComputePass::EncodeCommands() const { | |||
if (grid_size_.IsEmpty() || thread_group_size_.IsEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto about the validation logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment - I think making this a fatal error is too strong.
…/engine#40769) (#123801) Commit: 4a7fca08d9396ba00b436d1224192afd62adab85
…0769) (flutter#123801) Roll Flutter Engine from 7d190aa0e19f to 35507f9e91a7 (1 revision)
On Metal, this will trigger a runtime assert if not guarded against further down. We should avoid doing the dispatch at all if the grid size is zero in any dimension.