Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1304843005:
Deal with AllocateGpuMemoryBuffer returning null. (Closed)
|
Created:
5 years, 3 months ago by Daniele Castagna Modified:
5 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, posciak+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDeal with AllocateGpuMemoryBuffer returning null.
AllocateGpuMemoryBuffer can return null (e.g: when the GPU process is down).
This CL makes sure we don't crash in that case and make the code behave in
the most similar way to when allocations succeed.
BUG=529088
Committed: https://crrev.com/f894f228913000fc9ada3e3c9d0e71300a59f56a
Cr-Commit-Position: refs/heads/master@{#348738}
Patch Set 1 #Patch Set 2 : s/fail_to_allocated/fail_to_allocate #
Total comments: 2
Patch Set 3 : Address reveman's comment. #
Total comments: 4
Patch Set 4 : Remove outdated comment. #Patch Set 5 : Address reveman's nits. #
Total comments: 4
Patch Set 6 : Add "ForTesting" suffix. #
Messages
Total messages: 24 (5 generated)
dcastagna@chromium.org changed reviewers: + reveman@chromium.org
PTAL.
https://codereview.chromium.org/1304843005/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1304843005/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:540: return false; Not needed as discussed.
https://codereview.chromium.org/1304843005/diff/20001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1304843005/diff/20001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:540: return false; On 2015/09/09 at 22:45:28, reveman wrote: > Not needed as discussed. Done. Also removed the second part of the test that depended on this.
lgtm with nits https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:536: } is this just being moved from the header? necessary? https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right): https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:261: TEST_F(GpuMemoryBufferVideoFramePoolTest, AllocateGMBFail) { nit: s/AllocateGMBFail/FailToAllocateGpuMemoryBuffer/ to not abbreviate and have the name be consistent SetFailToAllocateGpuMemoryBuffer.
dcastagna@chromium.org changed reviewers: + dalecurtis@chromium.org
+dalecurtis for owner approval. https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:536: } On 2015/09/10 at 00:09:12, reveman wrote: > is this just being moved from the header? necessary? Nop. https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right): https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:261: TEST_F(GpuMemoryBufferVideoFramePoolTest, AllocateGMBFail) { On 2015/09/10 at 00:09:12, reveman wrote: > nit: s/AllocateGMBFail/FailToAllocateGpuMemoryBuffer/ to not abbreviate and have the name be consistent SetFailToAllocateGpuMemoryBuffer. Done.
On 2015/09/10 00:15:35, Daniele Castagna wrote: > +dalecurtis for owner approval. > > https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... > File media/video/gpu_memory_buffer_video_frame_pool.cc (right): > > https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... > media/video/gpu_memory_buffer_video_frame_pool.cc:536: } > On 2015/09/10 at 00:09:12, reveman wrote: > > is this just being moved from the header? necessary? > > Nop. > > https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... > File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right): > > https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... > media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:261: > TEST_F(GpuMemoryBufferVideoFramePoolTest, AllocateGMBFail) { > On 2015/09/10 at 00:09:12, reveman wrote: > > nit: s/AllocateGMBFail/FailToAllocateGpuMemoryBuffer/ to not abbreviate and > have the name be consistent SetFailToAllocateGpuMemoryBuffer. > > Done. Is this ready to land?
On 2015/09/12 at 23:42:32, ccameron wrote: > On 2015/09/10 00:15:35, Daniele Castagna wrote: > > +dalecurtis for owner approval. > > > > https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... > > File media/video/gpu_memory_buffer_video_frame_pool.cc (right): > > > > https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... > > media/video/gpu_memory_buffer_video_frame_pool.cc:536: } > > On 2015/09/10 at 00:09:12, reveman wrote: > > > is this just being moved from the header? necessary? > > > > Nop. > > > > https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... > > File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right): > > > > https://codereview.chromium.org/1304843005/diff/40001/media/video/gpu_memory_... > > media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:261: > > TEST_F(GpuMemoryBufferVideoFramePoolTest, AllocateGMBFail) { > > On 2015/09/10 at 00:09:12, reveman wrote: > > > nit: s/AllocateGMBFail/FailToAllocateGpuMemoryBuffer/ to not abbreviate and > > have the name be consistent SetFailToAllocateGpuMemoryBuffer. > > > > Done. > > Is this ready to land? It's still missing owner's LGTM.
There's a lot of conditionals added in this CL for what seems like an issue with a single root cause. Can you elaborate on why it's necessary to add all these conditionals instead of just null checking the initial creation (which is seemingly already done?) https://codereview.chromium.org/1304843005/diff/80001/media/renderers/mock_gp... File media/renderers/mock_gpu_video_accelerator_factories.h (right): https://codereview.chromium.org/1304843005/diff/80001/media/renderers/mock_gp... media/renderers/mock_gpu_video_accelerator_factories.h:64: void SetFailToAllocateGpuMemoryBuffer(bool fail) { Add ForTesting to the name so presubmit checks will catch invalid uses of this. https://codereview.chromium.org/1304843005/diff/80001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1304843005/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:358: if (!frame_resources) { I don't understand how this crashed before? Shouldn't this line have protected us against a failed allocation?
Patchset #6 (id:100001) has been deleted
On 2015/09/13 at 18:48:47, dalecurtis wrote: > There's a lot of conditionals added in this CL for what seems like an issue with a single root cause. Can you elaborate on why it's necessary to add all these conditionals instead of just null checking the initial creation (which is seemingly already done?) The initial allocation might succeed while subsequent GMB allocations might fail. In case any GMB allocation fails, the idea is to continue to do everything as if the allocation succeeded (e.g: create textures, create mailboxes). We have to avoid the places where the GMB is needed, thus all the conditionals. This is consistent with what the compositor is doing. https://codereview.chromium.org/1304843005/diff/80001/media/renderers/mock_gp... File media/renderers/mock_gpu_video_accelerator_factories.h (right): https://codereview.chromium.org/1304843005/diff/80001/media/renderers/mock_gp... media/renderers/mock_gpu_video_accelerator_factories.h:64: void SetFailToAllocateGpuMemoryBuffer(bool fail) { On 2015/09/13 at 18:48:47, DaleCurtis wrote: > Add ForTesting to the name so presubmit checks will catch invalid uses of this. Done. https://codereview.chromium.org/1304843005/diff/80001/media/video/gpu_memory_... File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1304843005/diff/80001/media/video/gpu_memory_... media/video/gpu_memory_buffer_video_frame_pool.cc:358: if (!frame_resources) { On 2015/09/13 at 18:48:47, DaleCurtis wrote: > I don't understand how this crashed before? Shouldn't this line have protected us against a failed allocation? What I think was happening before is the frame_resources allocation worked, but AllocateGpuMemoryBuffer returned nullptr. That caused an access violation later when we try to call Map on a nullptr in CopyVideoFrameToGpuMemoryBuffers.
On 2015/09/13 19:31:06, Daniele Castagna wrote: > On 2015/09/13 at 18:48:47, dalecurtis wrote: > > There's a lot of conditionals added in this CL for what seems like an issue > with a single root cause. Can you elaborate on why it's necessary to add all > these conditionals instead of just null checking the initial creation (which is > seemingly already done?) > > The initial allocation might succeed while subsequent GMB allocations might > fail. > In case any GMB allocation fails, the idea is to continue to do everything as if > the allocation succeeded (e.g: create textures, create mailboxes). We have to > avoid the places where the GMB is needed, thus all the conditionals. Why do we want to continue doing all those things? Are those steps reusable later? Otherwise I don't follow why we don't add one additional null-check to the call to allocate. > > This is consistent with what the compositor is doing. > > https://codereview.chromium.org/1304843005/diff/80001/media/renderers/mock_gp... > File media/renderers/mock_gpu_video_accelerator_factories.h (right): > > https://codereview.chromium.org/1304843005/diff/80001/media/renderers/mock_gp... > media/renderers/mock_gpu_video_accelerator_factories.h:64: void > SetFailToAllocateGpuMemoryBuffer(bool fail) { > On 2015/09/13 at 18:48:47, DaleCurtis wrote: > > Add ForTesting to the name so presubmit checks will catch invalid uses of > this. > > Done. > > https://codereview.chromium.org/1304843005/diff/80001/media/video/gpu_memory_... > File media/video/gpu_memory_buffer_video_frame_pool.cc (right): > > https://codereview.chromium.org/1304843005/diff/80001/media/video/gpu_memory_... > media/video/gpu_memory_buffer_video_frame_pool.cc:358: if (!frame_resources) { > On 2015/09/13 at 18:48:47, DaleCurtis wrote: > > I don't understand how this crashed before? Shouldn't this line have protected > us against a failed allocation? > > What I think was happening before is the frame_resources allocation worked, but > AllocateGpuMemoryBuffer returned nullptr. > That caused an access violation later when we try to call Map on a nullptr in > CopyVideoFrameToGpuMemoryBuffers.
On 2015/09/14 at 16:06:15, dalecurtis wrote: > On 2015/09/13 19:31:06, Daniele Castagna wrote: > > On 2015/09/13 at 18:48:47, dalecurtis wrote: > > > There's a lot of conditionals added in this CL for what seems like an issue > > with a single root cause. Can you elaborate on why it's necessary to add all > > these conditionals instead of just null checking the initial creation (which is > > seemingly already done?) > > > > The initial allocation might succeed while subsequent GMB allocations might > > fail. > > In case any GMB allocation fails, the idea is to continue to do everything as if > > the allocation succeeded (e.g: create textures, create mailboxes). We have to > > avoid the places where the GMB is needed, thus all the conditionals. > > Why do we want to continue doing all those things? Are those steps reusable later? > Otherwise I don't follow why we don't add one additional null-check to the call > to allocate. The general rule is that we should minimize the change in behavior during lost context situations. The more we diverge in behavior in these situations the more likely we are to introduce bugs as that code path will rarely be exercised. The GLES2Interface is still functional and any resources we might have created will need to be cleaned up as if the context was not lost.
On 2015/09/14 16:57:48, reveman wrote: > On 2015/09/14 at 16:06:15, dalecurtis wrote: > > On 2015/09/13 19:31:06, Daniele Castagna wrote: > > > On 2015/09/13 at 18:48:47, dalecurtis wrote: > > > > There's a lot of conditionals added in this CL for what seems like an > issue > > > with a single root cause. Can you elaborate on why it's necessary to add all > > > these conditionals instead of just null checking the initial creation (which > is > > > seemingly already done?) > > > > > > The initial allocation might succeed while subsequent GMB allocations might > > > fail. > > > In case any GMB allocation fails, the idea is to continue to do everything > as if > > > the allocation succeeded (e.g: create textures, create mailboxes). We have > to > > > avoid the places where the GMB is needed, thus all the conditionals. > > > > Why do we want to continue doing all those things? Are those steps reusable > later? > > Otherwise I don't follow why we don't add one additional null-check to the > call > > to allocate. > > The general rule is that we should minimize the change in behavior during lost > context situations. The more we diverge in behavior in these situations the more > likely we are to introduce bugs as that code path will rarely be exercised. The > GLES2Interface is still functional and any resources we might have created will > need to be cleaned up as if the context was not lost. You're worried about lines l.570 to l.583? Can we just allocate the GMB ahead of that section and return nullptr if allocation fails or destroy the GMB if the subsequent section fails?
On 2015/09/14 at 18:33:34, dalecurtis wrote: > On 2015/09/14 16:57:48, reveman wrote: > > On 2015/09/14 at 16:06:15, dalecurtis wrote: > > > On 2015/09/13 19:31:06, Daniele Castagna wrote: > > > > On 2015/09/13 at 18:48:47, dalecurtis wrote: > > > > > There's a lot of conditionals added in this CL for what seems like an > > issue > > > > with a single root cause. Can you elaborate on why it's necessary to add all > > > > these conditionals instead of just null checking the initial creation (which > > is > > > > seemingly already done?) > > > > > > > > The initial allocation might succeed while subsequent GMB allocations might > > > > fail. > > > > In case any GMB allocation fails, the idea is to continue to do everything > > as if > > > > the allocation succeeded (e.g: create textures, create mailboxes). We have > > to > > > > avoid the places where the GMB is needed, thus all the conditionals. > > > > > > Why do we want to continue doing all those things? Are those steps reusable > > later? > > > Otherwise I don't follow why we don't add one additional null-check to the > > call > > > to allocate. > > > > The general rule is that we should minimize the change in behavior during lost > > context situations. The more we diverge in behavior in these situations the more > > likely we are to introduce bugs as that code path will rarely be exercised. The > > GLES2Interface is still functional and any resources we might have created will > > need to be cleaned up as if the context was not lost. > > You're worried about lines l.570 to l.583? Can we just allocate the GMB ahead of > that section and return nullptr if allocation fails or destroy the GMB if the > subsequent section fails? My worry is not so much with the current code but future code. media's use of GLES2Interface doesn't handle lost context situations. We need to fix that asap. The general idea of using as much of the same code as possible in lost context situations might not make a lot of sense when handling of these situations is missing but it will be important as soon as we've implemented that.
On 2015/09/14 18:45:54, reveman wrote: > On 2015/09/14 at 18:33:34, dalecurtis wrote: > > On 2015/09/14 16:57:48, reveman wrote: > > > On 2015/09/14 at 16:06:15, dalecurtis wrote: > > > > On 2015/09/13 19:31:06, Daniele Castagna wrote: > > > > > On 2015/09/13 at 18:48:47, dalecurtis wrote: > > > > > > There's a lot of conditionals added in this CL for what seems like an > > > issue > > > > > with a single root cause. Can you elaborate on why it's necessary to add > all > > > > > these conditionals instead of just null checking the initial creation > (which > > > is > > > > > seemingly already done?) > > > > > > > > > > The initial allocation might succeed while subsequent GMB allocations > might > > > > > fail. > > > > > In case any GMB allocation fails, the idea is to continue to do > everything > > > as if > > > > > the allocation succeeded (e.g: create textures, create mailboxes). We > have > > > to > > > > > avoid the places where the GMB is needed, thus all the conditionals. > > > > > > > > Why do we want to continue doing all those things? Are those steps > reusable > > > later? > > > > Otherwise I don't follow why we don't add one additional null-check to the > > > call > > > > to allocate. > > > > > > The general rule is that we should minimize the change in behavior during > lost > > > context situations. The more we diverge in behavior in these situations the > more > > > likely we are to introduce bugs as that code path will rarely be exercised. > The > > > GLES2Interface is still functional and any resources we might have created > will > > > need to be cleaned up as if the context was not lost. > > > > You're worried about lines l.570 to l.583? Can we just allocate the GMB ahead > of > > that section and return nullptr if allocation fails or destroy the GMB if the > > subsequent section fails? > > My worry is not so much with the current code but future code. media's use of > GLES2Interface doesn't handle lost context situations. We need to fix that asap. > The general idea of using as much of the same code as possible in lost context > situations might not make a lot of sense when handling of these situations is > missing but it will be important as soon as we've implemented that. Do you think it's so broken that we'd do something entirely different with today's code? I.e., does it make sense to add several conditionals for a future potential path versus just simplifying this to a single return path (i.e. no worry of branching failures) for now? It seems we could easily add these conditionals later when that became the case. Further adding them now for a path which is unclear may just be increasing the size of our error surface. I defer to your judgement in either case, so lgtm.
On 2015/09/14 at 18:51:45, dalecurtis wrote: > On 2015/09/14 18:45:54, reveman wrote: > > On 2015/09/14 at 18:33:34, dalecurtis wrote: > > > On 2015/09/14 16:57:48, reveman wrote: > > > > On 2015/09/14 at 16:06:15, dalecurtis wrote: > > > > > On 2015/09/13 19:31:06, Daniele Castagna wrote: > > > > > > On 2015/09/13 at 18:48:47, dalecurtis wrote: > > > > > > > There's a lot of conditionals added in this CL for what seems like an > > > > issue > > > > > > with a single root cause. Can you elaborate on why it's necessary to add > > all > > > > > > these conditionals instead of just null checking the initial creation > > (which > > > > is > > > > > > seemingly already done?) > > > > > > > > > > > > The initial allocation might succeed while subsequent GMB allocations > > might > > > > > > fail. > > > > > > In case any GMB allocation fails, the idea is to continue to do > > everything > > > > as if > > > > > > the allocation succeeded (e.g: create textures, create mailboxes). We > > have > > > > to > > > > > > avoid the places where the GMB is needed, thus all the conditionals. > > > > > > > > > > Why do we want to continue doing all those things? Are those steps > > reusable > > > > later? > > > > > Otherwise I don't follow why we don't add one additional null-check to the > > > > call > > > > > to allocate. > > > > > > > > The general rule is that we should minimize the change in behavior during > > lost > > > > context situations. The more we diverge in behavior in these situations the > > more > > > > likely we are to introduce bugs as that code path will rarely be exercised. > > The > > > > GLES2Interface is still functional and any resources we might have created > > will > > > > need to be cleaned up as if the context was not lost. > > > > > > You're worried about lines l.570 to l.583? Can we just allocate the GMB ahead > > of > > > that section and return nullptr if allocation fails or destroy the GMB if the > > > subsequent section fails? > > > > My worry is not so much with the current code but future code. media's use of > > GLES2Interface doesn't handle lost context situations. We need to fix that asap. > > The general idea of using as much of the same code as possible in lost context > > situations might not make a lot of sense when handling of these situations is > > missing but it will be important as soon as we've implemented that. > > Do you think it's so broken that we'd do something entirely different with today's code? I.e., does it make sense to add several conditionals for a future potential path versus just simplifying this to a single return path (i.e. no worry of branching failures) for now? It seems we could easily add these conditionals later when that became the case. Further adding them now for a path which is unclear may just be increasing the size of our error surface. > > I defer to your judgement in either case, so lgtm. I don't feel strongly about the exact location of these conditionals. My general advice is to try execute as much of the code as possible in lost context situations. The important part is that we keep producing frames for the compositor as that needs to work correctly even if GMB allocation fails. Lost context detection is racy by design and if we stop producing frames as soon as we detect a lost context situation then all we're doing is reducing the ability to reproduce bugs that will only be triggered in these situations.
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1304843005/#ps120001 (title: "Add "ForTesting" suffix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1304843005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1304843005/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f894f228913000fc9ada3e3c9d0e71300a59f56a Cr-Commit-Position: refs/heads/master@{#348738}
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f894f228913000fc9ada3e3c9d0e71300a59f56a Cr-Commit-Position: refs/heads/master@{#348738} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
