Hi, @cyanglaz , It seems that the test DiscardResubmittedLayerTreeOnResize also needs to tweak

@ColdPaleLight Is DiscardResubmittedLayerTreeOnResize also failing like this? Is there an issue or log that I can refer to?

@ColdPaleLight Is DiscardResubmittedLayerTreeOnResize also failing like this? Is there an issue or log that I can refer to?

DiscardResubmittedLayerTreeOnResize is copied from DiscardLayerTreeOnResize and then modified, so they both have a lot of the same logic. I'm not sure if it have the same issue though.

@ColdPaleLight done, please review it to see if the updated test still makes sense.

I’m sorry, but I think updated test DiscardResubmittedLayerTreeOnResize is not quite right. This test was introduced in this PR #28474. It ensures that the discard_callback is passed to Rasterizer::Draw as a param when the frame needs to be resubmitted.

I tried removing the Thread merging logic at DiscardResubmittedLayerTreeOnResize today, but I didn't succeed.

@ColdPaleLight Could you elaborate what needs to be updated for test. It seems to me the current version of the test in this PR makes sure the frame is dropped when the original_size is used, which was the same as the previous version, just without thread merging.

@ColdPaleLight Could you elaborate what needs to be updated for test. It seems to me the current version of the test in this PR makes sure the frame is dropped when the original_size is used, which was the same as the previous version, just without thread merging.

Sorry, I didn't express clearly. This test is to ensure that the discard_callback here is passed to draw when consume_result is PipelineConsumeResult::MoreAvailable

So if someone accidentally makes the following change, the test will fail. You can make the following changes and see if the new version of the test fails, thank you.

--- a/shell/common/rasterizer.cc
+++ b/shell/common/rasterizer.cc
@@ -220,8 +220,7 @@ RasterStatus Rasterizer::Draw(
                resubmit_recorder = std::move(resubmit_recorder),
                discard_callback = std::move(discard_callback)]() mutable {
                 if (weak_this) {
-                  weak_this->Draw(std::move(resubmit_recorder), pipeline,
-                                  std::move(discard_callback));
+                  weak_this->Draw(std::move(resubmit_recorder), pipeline);
                 }
               }));
       break;

@ColdPaleLight Ah I see. I think in the resubmit case, thread merging is required ATM because raster_thread_merger_ is required for re-sbumit. See: https://github.com/flutter/engine/blob/main/flow/compositor_context.cc#L116-L123

The test looks fine and we didn't have any flake report so I'd like to keep it there for now.

If in the future, we have a case where we need to resubmit the tree in a embedder that doesn't require thread merging, then it makes sense to create a new similar test but without thread merging.

Does it make sense? I'll remove the change in DiscardResubmittedLayerTreeOnResize if you agree.

@ColdPaleLight Ah I see. I think in the resubmit case, thread merging is required ATM because raster_thread_merger_ is required for re-sbumit. See: https://github.com/flutter/engine/blob/main/flow/compositor_context.cc#L116-L123

The test looks fine and we didn't have any flake report so I'd like to keep it there for now.

If in the future, we have a case where we need to resubmit the tree in a embedder that doesn't require thread merging, then it makes sense to create a new similar test but without thread merging.

Does it make sense? I'll remove the change in DiscardResubmittedLayerTreeOnResize if you agree.

Yeah, It sounds good to me :-)

@chinmaygarde Friendly ping :)