Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1993833002:
Serialize usage of SurfaceView between AVDA instances. (Closed)
|
Created:
4 years, 7 months ago by liberato (no reviews please) Modified:
4 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSerialize usage of SurfaceView between AVDA instances.
If a new AVDA instance replaces the existing one, and both use the
same SurfaceView (full screen), then there is a race between the old
one forgetting about the SurfaceView and the new one starting to use
it. This can happen, for example, if one changes the video element
src= during playback.
If the incoming AVDA tries to construct the MediaCodec before the
outgoing AVDA's MediaCodec has disconnected from the SurfaceView,
then the incoming AVDA can fail to MediaCodec::start() with
IllegalArgumentException (undocumented, it looks like).
This CL causes the incoming AVDA to wait until the outgoing AVDA
completes cleanup if it will use the same SurfaceView.
BUG=611502
Committed: https://crrev.com/59cd81d346a99198cdbaffca1aae9f3193b318fd
Cr-Commit-Position: refs/heads/master@{#394577}
Patch Set 1 #Patch Set 2 : comments. #
Total comments: 23
Patch Set 3 : cl feedback. #Patch Set 4 : switched initializers. #
Total comments: 12
Patch Set 5 : cl feedback. #
Messages
Total messages: 18 (6 generated)
Description was changed from ========== Serialize usage of SurfaceView between AVDA instances. If a new AVDA instance replaces the existing one, and both use the same SurfaceView (full screen), then there is a race between the old one forgetting about the SurfaceView and the new one starting to use it. This can happen, for example, if one changes the video element src= during playback. If the incoming AVDA tries to construct the MediaCodec before the outgoing AVDA's MediaCodec has disconnected from the SurfaceView, then the incoming AVDA can fail to MediaCodec::start() with IllegalArgumentException (undocumented, it looks like). This CL causes the incoming AVDA to wait until the outgoing AVDA completes cleanup if it will use the same SurfaceView. BUG=611502 ========== to ========== Serialize usage of SurfaceView between AVDA instances. If a new AVDA instance replaces the existing one, and both use the same SurfaceView (full screen), then there is a race between the old one forgetting about the SurfaceView and the new one starting to use it. This can happen, for example, if one changes the video element src= during playback. If the incoming AVDA tries to construct the MediaCodec before the outgoing AVDA's MediaCodec has disconnected from the SurfaceView, then the incoming AVDA can fail to MediaCodec::start() with IllegalArgumentException (undocumented, it looks like). This CL causes the incoming AVDA to wait until the outgoing AVDA completes cleanup if it will use the same SurfaceView. BUG=611502 ==========
liberato@chromium.org changed reviewers: + dalecurtis@chromium.org, watk@chromium.org
surprisingly less code than i expected. -fl
https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:259: // Nobody has to wait for no surface. Move above find? https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:272: if (iter->second.waiter != nullptr) { Can you explain this design choice? I.e. why would we want to do this versus a queue of waiters? https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:288: if (iter == surface_waiter_map_.end()) Should this dcheck? https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:296: if (iter->second.owner == avda) { Use early returns instead. https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:344: OwnerRecord() : owner(nullptr), waiter(nullptr) {} Constructor prevents this from being a POD type. I.e. you can't do a = {null, null}.
https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:259: // Nobody has to wait for no surface. On 2016/05/18 17:59:11, DaleCurtis wrote: > Move above find? Done. https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:272: if (iter->second.waiter != nullptr) { On 2016/05/18 17:59:11, DaleCurtis wrote: > Can you explain this design choice? I.e. why would we want to do this versus a > queue of waiters? because WMPI maintains at most one AVDA instance. if the surface id matches, then the most recent one should win and the others lose. https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:288: if (iter == surface_waiter_map_.end()) On 2016/05/18 17:59:11, DaleCurtis wrote: > Should this dcheck? no. this way, one simply calls Deallocate unconditionally. otherwise, one has to track whether one is a waiter or not inside AVDA. https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:296: if (iter->second.owner == avda) { On 2016/05/18 17:59:11, DaleCurtis wrote: > Use early returns instead. Done. https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:344: OwnerRecord() : owner(nullptr), waiter(nullptr) {} On 2016/05/18 17:59:11, DaleCurtis wrote: > Constructor prevents this from being a POD type. I.e. you can't do a = {null, > null}. hrm, i just tried brace-or-equal but that doesn't allow pod initialization either. i've switched to that because it looks nicer. if i can only get one, then i'd rather have nullptr initializers, since this is used in a map. is there a way to get both, without defining an unused two-arg constructor (if that even works)?
https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:272: if (iter->second.waiter != nullptr) { On 2016/05/18 at 18:49:39, liberato wrote: > On 2016/05/18 17:59:11, DaleCurtis wrote: > > Can you explain this design choice? I.e. why would we want to do this versus a > > queue of waiters? > > because WMPI maintains at most one AVDA instance. if the surface id matches, then the most recent one should win and the others lose. Hmm, what about other WMPI instances? https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:344: OwnerRecord() : owner(nullptr), waiter(nullptr) {} On 2016/05/18 at 18:49:39, liberato wrote: > On 2016/05/18 17:59:11, DaleCurtis wrote: > > Constructor prevents this from being a POD type. I.e. you can't do a = {null, > > null}. > > hrm, i just tried brace-or-equal but that doesn't allow pod initialization either. i've switched to that because it looks nicer. > > if i can only get one, then i'd rather have nullptr initializers, since this is used in a map. > > is there a way to get both, without defining an unused two-arg constructor (if that even works)? Dropping the constructor and not using = nullptr on the members is the only way to get a POD type. If you want null initialization (no reason for a map use case where you're looking up keys), then just use the = nullptr syntax instead of a constructor at least.
https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:272: if (iter->second.waiter != nullptr) { On 2016/05/18 18:55:49, DaleCurtis wrote: > On 2016/05/18 at 18:49:39, liberato wrote: > > On 2016/05/18 17:59:11, DaleCurtis wrote: > > > Can you explain this design choice? I.e. why would we want to do this versus > a > > > queue of waiters? > > > > because WMPI maintains at most one AVDA instance. if the surface id matches, > then the most recent one should win and the others lose. > > Hmm, what about other WMPI instances? |surface_id_| will not be the same if the surface is in use anywhere. in fact, it might just never be re-used. https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:344: OwnerRecord() : owner(nullptr), waiter(nullptr) {} On 2016/05/18 18:55:49, DaleCurtis wrote: > On 2016/05/18 at 18:49:39, liberato wrote: > > On 2016/05/18 17:59:11, DaleCurtis wrote: > > > Constructor prevents this from being a POD type. I.e. you can't do a = > {null, > > > null}. > > > > hrm, i just tried brace-or-equal but that doesn't allow pod initialization > either. i've switched to that because it looks nicer. > > > > if i can only get one, then i'd rather have nullptr initializers, since this > is used in a map. > > > > is there a way to get both, without defining an unused two-arg constructor (if > that even works)? > > Dropping the constructor and not using = nullptr on the members is the only way > to get a POD type. If you want null initialization (no reason for a map use case > where you're looking up keys), then just use the = nullptr syntax instead of a > constructor at least. i switched to "= nullptr" in the most recent CL. even for maps, i'd like to guarantee that the ptrs are initialized when adding a key.
lgtm https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:261: SurfaceWaiterMap::iterator iter = surface_waiter_map_.find(surface_id); Can use auto here if you like, ditto below. https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:262: Delete extra line. https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:265: surface_waiter_map_[surface_id].owner = avda; Change this to = { avda, nullptr } to avoid two lookups and two assigns. https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:272: if (iter->second.waiter != nullptr) { if (iter->second.waiter) https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:299: if (AndroidVideoDecodeAccelerator* waiter = iter->second.waiter) { Invert this so that it's if (!iter->second.waiter) { ...; return; }. https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:431: cdm_id_ = config.cdm_id; Seems like we should just keep a copy of config instead of all these single members.
lgtm with nits https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:252: // |owner| would like to use |surface_id|. If it is not busy, then mark it No "owner" any more https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:342: // [surface id] = accelerator that owns the surface, or nullptr. Comment seems out of date/wrong place https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:508: if (!InitializeStrategy()) { Could merge this block into the above with a || if you want https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.h (right): https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.h:223: bool InitializeStrategy(); Header order doesn't match cc order
https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:252: // |owner| would like to use |surface_id|. If it is not busy, then mark it On 2016/05/18 20:38:18, watk wrote: > No "owner" any more Done. https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:342: // [surface id] = accelerator that owns the surface, or nullptr. On 2016/05/18 20:38:18, watk wrote: > Comment seems out of date/wrong place Done. https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:508: if (!InitializeStrategy()) { On 2016/05/18 20:38:18, watk wrote: > Could merge this block into the above with a || if you want Done. https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.h (right): https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.h:223: bool InitializeStrategy(); On 2016/05/18 20:38:18, watk wrote: > Header order doesn't match cc order not sure that i follow -- OnSurfaceAvailable preceeds InitializeStrategy. the rest of them are out of order, but i kept the new ones close to ::Initialize. couldn't put them close in the header since ::Initialize is inherited. https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:261: SurfaceWaiterMap::iterator iter = surface_waiter_map_.find(surface_id); On 2016/05/18 19:58:48, DaleCurtis wrote: > Can use auto here if you like, ditto below. Done. https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:262: On 2016/05/18 19:58:48, DaleCurtis wrote: > Delete extra line. Done. https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:265: surface_waiter_map_[surface_id].owner = avda; On 2016/05/18 19:58:48, DaleCurtis wrote: > Change this to = { avda, nullptr } to avoid two lookups and two assigns. i removed the second assignment, since |waiter| is initialized to nullptr anyway. https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:272: if (iter->second.waiter != nullptr) { On 2016/05/18 19:58:48, DaleCurtis wrote: > if (iter->second.waiter) Done. https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:299: if (AndroidVideoDecodeAccelerator* waiter = iter->second.waiter) { On 2016/05/18 19:58:48, DaleCurtis wrote: > Invert this so that it's if (!iter->second.waiter) { ...; return; }. Done. https://codereview.chromium.org/1993833002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:431: cdm_id_ = config.cdm_id; On 2016/05/18 19:58:48, DaleCurtis wrote: > Seems like we should just keep a copy of config instead of all these single > members. Done.
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1993833002/#ps80001 (title: "cl feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993833002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993833002/80001
https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.h (right): https://codereview.chromium.org/1993833002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.h:223: bool InitializeStrategy(); On 2016/05/18 21:13:13, liberato wrote: > On 2016/05/18 20:38:18, watk wrote: > > Header order doesn't match cc order > > not sure that i follow -- OnSurfaceAvailable preceeds InitializeStrategy. the > rest of them are out of order, but i kept the new ones close to ::Initialize. > > couldn't put them close in the header since ::Initialize is inherited. Sorry, I just noticed that InitializeStrategy is followed by DoIOTask so I thought you inserted them at the wrong place. But you did the right thing and the order was already weird.
Message was sent while issue was closed.
Description was changed from ========== Serialize usage of SurfaceView between AVDA instances. If a new AVDA instance replaces the existing one, and both use the same SurfaceView (full screen), then there is a race between the old one forgetting about the SurfaceView and the new one starting to use it. This can happen, for example, if one changes the video element src= during playback. If the incoming AVDA tries to construct the MediaCodec before the outgoing AVDA's MediaCodec has disconnected from the SurfaceView, then the incoming AVDA can fail to MediaCodec::start() with IllegalArgumentException (undocumented, it looks like). This CL causes the incoming AVDA to wait until the outgoing AVDA completes cleanup if it will use the same SurfaceView. BUG=611502 ========== to ========== Serialize usage of SurfaceView between AVDA instances. If a new AVDA instance replaces the existing one, and both use the same SurfaceView (full screen), then there is a race between the old one forgetting about the SurfaceView and the new one starting to use it. This can happen, for example, if one changes the video element src= during playback. If the incoming AVDA tries to construct the MediaCodec before the outgoing AVDA's MediaCodec has disconnected from the SurfaceView, then the incoming AVDA can fail to MediaCodec::start() with IllegalArgumentException (undocumented, it looks like). This CL causes the incoming AVDA to wait until the outgoing AVDA completes cleanup if it will use the same SurfaceView. BUG=611502 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Serialize usage of SurfaceView between AVDA instances. If a new AVDA instance replaces the existing one, and both use the same SurfaceView (full screen), then there is a race between the old one forgetting about the SurfaceView and the new one starting to use it. This can happen, for example, if one changes the video element src= during playback. If the incoming AVDA tries to construct the MediaCodec before the outgoing AVDA's MediaCodec has disconnected from the SurfaceView, then the incoming AVDA can fail to MediaCodec::start() with IllegalArgumentException (undocumented, it looks like). This CL causes the incoming AVDA to wait until the outgoing AVDA completes cleanup if it will use the same SurfaceView. BUG=611502 ========== to ========== Serialize usage of SurfaceView between AVDA instances. If a new AVDA instance replaces the existing one, and both use the same SurfaceView (full screen), then there is a race between the old one forgetting about the SurfaceView and the new one starting to use it. This can happen, for example, if one changes the video element src= during playback. If the incoming AVDA tries to construct the MediaCodec before the outgoing AVDA's MediaCodec has disconnected from the SurfaceView, then the incoming AVDA can fail to MediaCodec::start() with IllegalArgumentException (undocumented, it looks like). This CL causes the incoming AVDA to wait until the outgoing AVDA completes cleanup if it will use the same SurfaceView. BUG=611502 Committed: https://crrev.com/59cd81d346a99198cdbaffca1aae9f3193b318fd Cr-Commit-Position: refs/heads/master@{#394577} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/59cd81d346a99198cdbaffca1aae9f3193b318fd Cr-Commit-Position: refs/heads/master@{#394577} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
