|
|
Created:
6 years, 1 month ago by sandersd (OOO until July 31) Modified:
6 years ago Reviewers:
*DaleCurtis CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@vt_pic_order Project:
chromium Visibility:
Public. |
DescriptionRemove reliance on VideoToolbox frame ordering.
It seems that VideoToolbox occasionally reorders frames (in my testing, the fourth one keeps appearing third, and then there are a few more random swaps that stop by around frame 16). This CL makes that a non-fatal error.
BUG=133828
Committed: https://crrev.com/4bc4f40ebaf426df2b58f4ab94549c9cc29364a6
Cr-Commit-Position: refs/heads/master@{#305944}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Key by bitstream_id. #Patch Set 3 : Rebase. #Patch Set 4 : Fix pointer dereference. #
Messages
Total messages: 24 (8 generated)
sandersd@chromium.org changed reviewers: + dalecurtis@chromium.org
sandersd@chromium.org changed required reviewers: + dalecurtis@chromium.org
https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.h:178: std::map<Frame*, linked_ptr<Frame>> pending_frames_; Why not just an std::set?
The good news is that frames do not seem to be getting silently dropped! https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.h:178: std::map<Frame*, linked_ptr<Frame>> pending_frames_; On 2014/11/22 00:10:51, DaleCurtis wrote: > Why not just an std::set? The linked_ptr is keeping the object alive in this situation, but I need to be able to find that ownership by raw pointer. (Reworking this ownership model again is planned, but probably comes after initial rollout, testing, and bugs.)
https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.h:178: std::map<Frame*, linked_ptr<Frame>> pending_frames_; On 2014/11/22 00:20:40, sandersd wrote: > On 2014/11/22 00:10:51, DaleCurtis wrote: > > Why not just an std::set? > > The linked_ptr is keeping the object alive in this situation, but I need to be > able to find that ownership by raw pointer. > > (Reworking this ownership model again is planned, but probably comes after > initial rollout, testing, and bugs.) This is really nasty though. Are you really saving yourself any work by not just having a set<Frame*> and transferring ownership into a scoper inside the task?
https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.h:178: std::map<Frame*, linked_ptr<Frame>> pending_frames_; On 2014/11/22 00:25:06, DaleCurtis wrote: > On 2014/11/22 00:20:40, sandersd wrote: > > On 2014/11/22 00:10:51, DaleCurtis wrote: > > > Why not just an std::set? > > > > The linked_ptr is keeping the object alive in this situation, but I need to be > > able to find that ownership by raw pointer. > > > > (Reworking this ownership model again is planned, but probably comes after > > initial rollout, testing, and bugs.) > > This is really nasty though. Are you really saving yourself any work by not just > having a set<Frame*> and transferring ownership into a scoper inside the task? No, but inside the task I'd have to have this same structure, because I need to be able to save the reference while the frame is owned by VideoToolbox. As per the discussion in the work queue CL, that's no so bad if I trust VideoToolbox to return every frame, as we can just release the ownership instead, but Pawel didn't want to go for that option, so for now I'm changing as little code as possible. (It's not too late though, if it's really important we can get this done properly right away.)
https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.h:178: std::map<Frame*, linked_ptr<Frame>> pending_frames_; On 2014/11/22 00:32:48, sandersd wrote: > On 2014/11/22 00:25:06, DaleCurtis wrote: > > On 2014/11/22 00:20:40, sandersd wrote: > > > On 2014/11/22 00:10:51, DaleCurtis wrote: > > > > Why not just an std::set? > > > > > > The linked_ptr is keeping the object alive in this situation, but I need to > be > > > able to find that ownership by raw pointer. > > > > > > (Reworking this ownership model again is planned, but probably comes after > > > initial rollout, testing, and bugs.) > > > > This is really nasty though. Are you really saving yourself any work by not > just > > having a set<Frame*> and transferring ownership into a scoper inside the task? > > No, but inside the task I'd have to have this same structure, because I need to > be able to save the reference while the frame is owned by VideoToolbox. > > As per the discussion in the work queue CL, that's no so bad if I trust > VideoToolbox to return every frame, as we can just release the ownership > instead, but Pawel didn't want to go for that option, so for now I'm changing as > little code as possible. (It's not too late though, if it's really important we > can get this done properly right away.) I don't follow why you'd change the tasks... they already contain a linked_ptr, so you really just need the raw ptr set between Decode and DecodeDone... At destruction / stop / whatever you can nuke incomplete frames.
https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.h:178: std::map<Frame*, linked_ptr<Frame>> pending_frames_; On 2014/11/22 00:53:40, DaleCurtis wrote: > On 2014/11/22 00:32:48, sandersd wrote: > > On 2014/11/22 00:25:06, DaleCurtis wrote: > > > On 2014/11/22 00:20:40, sandersd wrote: > > > > On 2014/11/22 00:10:51, DaleCurtis wrote: > > > > > Why not just an std::set? > > > > > > > > The linked_ptr is keeping the object alive in this situation, but I need > to > > be > > > > able to find that ownership by raw pointer. > > > > > > > > (Reworking this ownership model again is planned, but probably comes after > > > > initial rollout, testing, and bugs.) > > > > > > This is really nasty though. Are you really saving yourself any work by not > > just > > > having a set<Frame*> and transferring ownership into a scoper inside the > task? > > > > No, but inside the task I'd have to have this same structure, because I need > to > > be able to save the reference while the frame is owned by VideoToolbox. > > > > As per the discussion in the work queue CL, that's no so bad if I trust > > VideoToolbox to return every frame, as we can just release the ownership > > instead, but Pawel didn't want to go for that option, so for now I'm changing > as > > little code as possible. (It's not too late though, if it's really important > we > > can get this done properly right away.) > > I don't follow why you'd change the tasks... they already contain a linked_ptr, > so you really just need the raw ptr set between Decode and DecodeDone... At > destruction / stop / whatever you can nuke incomplete frames. Sorry, that's DecodeTask().
https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/753713002/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.h:178: std::map<Frame*, linked_ptr<Frame>> pending_frames_; On 2014/11/22 00:55:50, sandersd wrote: > On 2014/11/22 00:53:40, DaleCurtis wrote: > > On 2014/11/22 00:32:48, sandersd wrote: > > > On 2014/11/22 00:25:06, DaleCurtis wrote: > > > > On 2014/11/22 00:20:40, sandersd wrote: > > > > > On 2014/11/22 00:10:51, DaleCurtis wrote: > > > > > > Why not just an std::set? > > > > > > > > > > The linked_ptr is keeping the object alive in this situation, but I need > > to > > > be > > > > > able to find that ownership by raw pointer. > > > > > > > > > > (Reworking this ownership model again is planned, but probably comes > after > > > > > initial rollout, testing, and bugs.) > > > > > > > > This is really nasty though. Are you really saving yourself any work by > not > > > just > > > > having a set<Frame*> and transferring ownership into a scoper inside the > > task? > > > > > > No, but inside the task I'd have to have this same structure, because I need > > to > > > be able to save the reference while the frame is owned by VideoToolbox. > > > > > > As per the discussion in the work queue CL, that's no so bad if I trust > > > VideoToolbox to return every frame, as we can just release the ownership > > > instead, but Pawel didn't want to go for that option, so for now I'm > changing > > as > > > little code as possible. (It's not too late though, if it's really important > > we > > > can get this done properly right away.) > > > > I don't follow why you'd change the tasks... they already contain a > linked_ptr, > > so you really just need the raw ptr set between Decode and DecodeDone... At > > destruction / stop / whatever you can nuke incomplete frames. > > Sorry, that's DecodeTask(). I still don't follow your arguments against set<Frame*> or set<linked_ptr<Frame*>> here; note: you can set a custom comparator on the set. A map of Frame* to linked_ptr<Frame> is such a strong code smell that I'm not convinced there's not a better way here. If those Cls are still in flight they should be changed.
lgtm, given you intend to refactor this for scopers later.
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/753713002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/87085)
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/753713002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/753713002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4bc4f40ebaf426df2b58f4ab94549c9cc29364a6 Cr-Commit-Position: refs/heads/master@{#305944} |