Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Revert "[Impeller] fix line/polygon depth and GLES scissor state. (#56494)" #56551

Closed
wants to merge 1 commit into from

Conversation

chinmaygarde
Copy link
Member

@chinmaygarde chinmaygarde commented Nov 13, 2024

This reverts commit 950e240 in #56494.

This caused a regression in a customer application (@lyceel).

I am not sure I should revert this whole patch. The goldens don't change if I just revert the patches to render_pass_gles.cc. So while that part was untested in the original commit, the other patches should be good. The original commit message did mention two separate issues were being addressed.

I am working on reduced test case with just the scissor regression with the customer.

…utter#56494)"

This reverts commit 950e240.

This caused a regression in a customer application (@lyceel).

I am not sure I should revert this whole patch. The goldens don't change if I just revert the patches to `render_pass_gles.cc`. So while that part was untested in the original commit, the other patches should be good. The original commit message did mention two separate issues were being addressed.

I am working on reduced test case with just the scissor regression with the customer.
@chinmaygarde
Copy link
Member Author

chinmaygarde commented Nov 13, 2024

From the original commit message.

Fixed in the same PR because the golden actually demonstrates both problems with GLES, and luckily I found 2. when I noticed 1.

If I back out just the disabling of the scissor test, the goldens still look identical to Metal/VK. Am I missing something?

Screenshot 2024-11-12 at 5 24 21 PM

@jonahwilliams
Copy link
Member

I don't see an obvious way for the stencil setting to leak out of the render pass. Even if we leave it set, then a subsequent render pass would immediately disable it via gl.Disable(GL_SCISSOR_TEST) (at least until a new stencil value was set).

Is this bug happening because the stencil state is leaking into the host context libimpeller is embedded into?

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Would like to see a repro, otherwise we'd just need to reland this to unblock Android GLES

@chinmaygarde
Copy link
Member Author

Yeah, working on a repro. Found this regression late in the day.

otherwise we'd just need to reland this to unblock Android GLES

Thats the thing. The goldens don't change even if I just revert just the removal of the scissor test toggle. So, I was wondering if you added it by mistake. Either way, its not tested.

But let's revisit this in the morning. The reduced test case ought to be simple enough. And I haven't reasoned about the embedder either. So it could be that.

@chinmaygarde
Copy link
Member Author

Spoke about this in person and the regression may be because of #56597. Either way, closing this.

@chinmaygarde chinmaygarde deleted the revertscissor branch November 14, 2024 22:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants