Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2664943002:
Mark Layer Damaged When The Client Is Gone (Closed)
DescriptionMark Layer Damaged When The Client Is Gone
We found a bug that black boxes are seen when hangouts switches focused
participants. It is related to an issue that we did not mark the layer
hosting WebMediaPlayerMS damaged properly. This CL fixed that issue.
BUG=681006
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2664943002
Cr-Commit-Position: refs/heads/master@{#447811}
Committed: https://chromium.googlesource.com/chromium/src/+/101569dac2a9cee88240aa87fc695b9b6fb93aa1
Patch Set 1 #Patch Set 2 : Format #Patch Set 3 : Unittest #
Total comments: 2
Patch Set 4 : Add test #
Messages
Total messages: 36 (14 generated)
Description was changed from ========== Mark Damage: Experimental: BUG= ========== to ========== Mark Damage: Experimental: BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Mark Damage: Experimental: BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Mark Layer Damaged When The Client Is Gone We found a bug that black boxes are seen when hangouts switches focused participants. It is related to an issue that we did not mark the layer hosting WebMediaPlayerMS damaged properly. This CL fixed that issue. BUG=681006 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
qiangchen@chromium.org changed reviewers: + danakj@chromium.org
Can you take a look?
Can you add tests?
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/30 22:09:47, danakj (slow) wrote: > Can you add tests? Sure, then I'll need to do some more research on existing test cases.
On Mon, Jan 30, 2017 at 5:16 PM, <qiangchen@chromium.org> wrote: > On 2017/01/30 22:09:47, danakj (slow) wrote: > > Can you add tests? > > Sure, then I'll need to do some more research on existing test cases. > No problem, I saw there alrady some tests that verify SetNeedsRedraw is called in the corresponding unit test file, so have a look thru. > > https://codereview.chromium.org/2664943002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/30 22:17:52, danakj (slow) wrote: > On Mon, Jan 30, 2017 at 5:16 PM, <mailto:qiangchen@chromium.org> wrote: > > > On 2017/01/30 22:09:47, danakj (slow) wrote: > > > Can you add tests? > > > > Sure, then I'll need to do some more research on existing test cases. > > > > No problem, I saw there alrady some tests that verify SetNeedsRedraw is > called in the corresponding unit test file, so have a look thru. > > > > > > https://codereview.chromium.org/2664943002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Can you be more specific? I am looking at VideoFrameProviderClientImplTest, and find not easy to track the call of SetNeedsRedraw. What unittest are you looking at? Thanks.
On 2017/01/30 23:50:08, danakj (slow) wrote: > https://cs.chromium.org/chromium/src/cc/layers/video_frame_provider_client_im... > was what I saw. Got it. So we can track update_rect to indirectly verify the call to SetNeedsRedraw. Thanks.
The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Add a unittest
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you take a look? Just a couple lines of change.
https://codereview.chromium.org/2664943002/diff/40001/cc/layers/video_frame_p... File cc/layers/video_frame_provider_client_impl_unittest.cc (right): https://codereview.chromium.org/2664943002/diff/40001/cc/layers/video_frame_p... cc/layers/video_frame_provider_client_impl_unittest.cc:109: StopRendering(); Can you also stop before start and see no damage happens? Can you also test StopUsingProvider()?
Added a test case. https://codereview.chromium.org/2664943002/diff/40001/cc/layers/video_frame_p... File cc/layers/video_frame_provider_client_impl_unittest.cc (right): https://codereview.chromium.org/2664943002/diff/40001/cc/layers/video_frame_p... cc/layers/video_frame_provider_client_impl_unittest.cc:109: StopRendering(); On 2017/02/01 17:52:09, danakj (slow) wrote: > Can you also stop before start and see no damage happens? > > Can you also test StopUsingProvider()? That does not work, because in StopRendering there is a DCHECK(rendering_), which will fail the test. Added test on StopUsingProvider
On 2017/02/01 18:52:00, qiangchen wrote: > Added a test case. > > https://codereview.chromium.org/2664943002/diff/40001/cc/layers/video_frame_p... > File cc/layers/video_frame_provider_client_impl_unittest.cc (right): > > https://codereview.chromium.org/2664943002/diff/40001/cc/layers/video_frame_p... > cc/layers/video_frame_provider_client_impl_unittest.cc:109: StopRendering(); > On 2017/02/01 17:52:09, danakj (slow) wrote: > > Can you also stop before start and see no damage happens? > > > > Can you also test StopUsingProvider()? > > That does not work, because in StopRendering there is a DCHECK(rendering_), > which will fail the test. > > Added test on StopUsingProvider Oh sorry you're right. It's the active_video_layer_ branch I was hoping to test. Otherwise it can be removed without any test failing?
On 2017/02/01 19:37:49, danakj (slow) wrote: > On 2017/02/01 18:52:00, qiangchen wrote: > > Added a test case. > > > > > https://codereview.chromium.org/2664943002/diff/40001/cc/layers/video_frame_p... > > File cc/layers/video_frame_provider_client_impl_unittest.cc (right): > > > > > https://codereview.chromium.org/2664943002/diff/40001/cc/layers/video_frame_p... > > cc/layers/video_frame_provider_client_impl_unittest.cc:109: StopRendering(); > > On 2017/02/01 17:52:09, danakj (slow) wrote: > > > Can you also stop before start and see no damage happens? > > > > > > Can you also test StopUsingProvider()? > > > > That does not work, because in StopRendering there is a DCHECK(rendering_), > > which will fail the test. > > > > Added test on StopUsingProvider > > Oh sorry you're right. It's the active_video_layer_ branch I was hoping to test. > Otherwise it can be removed without any test failing? The current code tests SetNeedsRedraw() call indirectly by detecting update rect. Without calling SetNeedsRedraw(), the newly added test cases will fail. So I think they are not trivial test cases.
On Wed, Feb 1, 2017 at 3:15 PM, <qiangchen@chromium.org> wrote: > On 2017/02/01 19:37:49, danakj (slow) wrote: > > On 2017/02/01 18:52:00, qiangchen wrote: > > > Added a test case. > > > > > > > > > https://codereview.chromium.org/2664943002/diff/40001/cc/ > layers/video_frame_provider_client_impl_unittest.cc > > > File cc/layers/video_frame_provider_client_impl_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/2664943002/diff/40001/cc/ > layers/video_frame_provider_client_impl_unittest.cc#newcode109 > > > cc/layers/video_frame_provider_client_impl_unittest.cc:109: > StopRendering(); > > > On 2017/02/01 17:52:09, danakj (slow) wrote: > > > > Can you also stop before start and see no damage happens? > > > > > > > > Can you also test StopUsingProvider()? > > > > > > That does not work, because in StopRendering there is a > DCHECK(rendering_), > > > which will fail the test. > > > > > > Added test on StopUsingProvider > > > > Oh sorry you're right. It's the active_video_layer_ branch I was hoping > to > test. > > Otherwise it can be removed without any test failing? > > The current code tests SetNeedsRedraw() call indirectly by detecting update > rect. > Without calling SetNeedsRedraw(), the newly added test cases will fail. > So I think they are not trivial test cases. > Sorry that's not what I mean. The tests both test when the active_video_layer_ branch is true. They don't test when it's false. Would any test fail if you removed that if-statement and always called SetNeedsRedraw() instead? Something should fail > https://codereview.chromium.org/2664943002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/01 20:20:33, danakj (slow) wrote: > On Wed, Feb 1, 2017 at 3:15 PM, <mailto:qiangchen@chromium.org> wrote: > > > On 2017/02/01 19:37:49, danakj (slow) wrote: > > > On 2017/02/01 18:52:00, qiangchen wrote: > > > > Added a test case. > > > > > > > > > > > > > https://codereview.chromium.org/2664943002/diff/40001/cc/ > > layers/video_frame_provider_client_impl_unittest.cc > > > > File cc/layers/video_frame_provider_client_impl_unittest.cc (right): > > > > > > > > > > > > > https://codereview.chromium.org/2664943002/diff/40001/cc/ > > layers/video_frame_provider_client_impl_unittest.cc#newcode109 > > > > cc/layers/video_frame_provider_client_impl_unittest.cc:109: > > StopRendering(); > > > > On 2017/02/01 17:52:09, danakj (slow) wrote: > > > > > Can you also stop before start and see no damage happens? > > > > > > > > > > Can you also test StopUsingProvider()? > > > > > > > > That does not work, because in StopRendering there is a > > DCHECK(rendering_), > > > > which will fail the test. > > > > > > > > Added test on StopUsingProvider > > > > > > Oh sorry you're right. It's the active_video_layer_ branch I was hoping > > to > > test. > > > Otherwise it can be removed without any test failing? > > > > The current code tests SetNeedsRedraw() call indirectly by detecting update > > rect. > > Without calling SetNeedsRedraw(), the newly added test cases will fail. > > So I think they are not trivial test cases. > > > > Sorry that's not what I mean. The tests both test when the > active_video_layer_ branch is true. They don't test when it's false. Would > any test fail if you removed that if-statement and always called > SetNeedsRedraw() instead? Something should fail > > > > > https://codereview.chromium.org/2664943002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Removing "if (active_video_layer_)" would crash many cases without setting the video layer, because in that case you call some member function for a null pointer. Or I can write a test case to verify SetNeedsRedraw does not get called when active_video_layer_ is not set, but I do think it is trivial, because no one can call a member function of nullptr.
I see what you mean. The code could change in the future to report damage in any other way, instead of going thru the layer. I saw the if as a logical we don't want to report damage in that case but I guess you see it as a we don't want to crash. I guess we can go with this, yeh. Thanks for the tests! LGTM
The CQ bit was checked by qiangchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486058202033750,
"parent_rev": "e51372ddbee311188504e30833e865c80623a9a0", "commit_rev":
"101569dac2a9cee88240aa87fc695b9b6fb93aa1"}
Message was sent while issue was closed.
Description was changed from ========== Mark Layer Damaged When The Client Is Gone We found a bug that black boxes are seen when hangouts switches focused participants. It is related to an issue that we did not mark the layer hosting WebMediaPlayerMS damaged properly. This CL fixed that issue. BUG=681006 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Mark Layer Damaged When The Client Is Gone We found a bug that black boxes are seen when hangouts switches focused participants. It is related to an issue that we did not mark the layer hosting WebMediaPlayerMS damaged properly. This CL fixed that issue. BUG=681006 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2664943002 Cr-Commit-Position: refs/heads/master@{#447811} Committed: https://chromium.googlesource.com/chromium/src/+/101569dac2a9cee88240aa87fc69... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/101569dac2a9cee88240aa87fc69...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2667383009/ by qiangchen@chromium.org. The reason for reverting is: Crash.
Message was sent while issue was closed.
Looks like Stop() should set the active layer to null before calling StopRendering()
Message was sent while issue was closed.
On 2017/02/06 19:57:28, danakj wrote: > Looks like Stop() should set the active layer to null before calling > StopRendering() If we do that, the black box issue will come back. Let me try the solution that marking damage when the video_layer_ is set.
Message was sent while issue was closed.
On Fri, Feb 10, 2017 at 2:45 PM, <qiangchen@chromium.org> wrote: > On 2017/02/06 19:57:28, danakj wrote: > > Looks like Stop() should set the active layer to null before calling > > StopRendering() > > If we do that, the black box issue will come back. > Let me try the solution that marking damage when the video_layer_ is set. > Will it? Stop() is called when the VideoLayerImpl is being destroyed. Is it called at other times? I tried to find but didn't. > > https://codereview.chromium.org/2664943002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
