Remove reliance on VideoToolbox frame ordering.

Remove 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}

[sandersd (OOO until July 31), 2014-11-22 00:07:06]

sandersd@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[sandersd (OOO until July 31), 2014-11-22 00:07:06]

sandersd@chromium.org changed required reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2014-11-22 00:10:51]

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?

[sandersd (OOO until July 31), 2014-11-22 00:20:40]

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.)

[DaleCurtis, 2014-11-22 00:25:06]

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?

[sandersd (OOO until July 31), 2014-11-22 00:32:48]

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.)

[DaleCurtis, 2014-11-22 00:53:40]

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.

[sandersd (OOO until July 31), 2014-11-22 00:55:50]

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().

[DaleCurtis, 2014-11-25 21:56:20]

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.

[DaleCurtis, 2014-11-25 22:34:37]

lgtm, given you intend to refactor this for scopers later.

[sandersd (OOO until July 31), 2014-11-27 00:33:31]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-11-27 00:34:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/753713002/40001

[commit-bot: I haz the power, 2014-11-27 00:54:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-27 00:54:39]

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)

[sandersd (OOO until July 31), 2014-11-27 01:00:55]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-11-27 01:01:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/753713002/60001

[commit-bot: I haz the power, 2014-11-27 01:23:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-27 01:23:06]

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...)

[sandersd (OOO until July 31), 2014-11-27 01:26:11]

Patchset #4 (id:60001) has been deleted

[sandersd (OOO until July 31), 2014-11-27 01:26:23]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-11-27 01:26:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/753713002/80001

[commit-bot: I haz the power, 2014-11-27 02:08:51]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-11-27 02:09:39]

Patchset 4 (id:??) landed as
https://crrev.com/4bc4f40ebaf426df2b58f4ab94549c9cc29364a6
Cr-Commit-Position: refs/heads/master@{#305944}