- 
                Notifications
    You must be signed in to change notification settings 
- Fork 6k
[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.
| 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)
| } | ||
|  | ||
| 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.