Remove reliance on VideoToolbox frame ordering.

content/common/gpu/media/vt_video_decode_accelerator.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_COMMON_GPU_MEDIA_VT_VIDEO_DECODE_ACCELERATOR_H_
 #define CONTENT_COMMON_GPU_MEDIA_VT_VIDEO_DECODE_ACCELERATOR_H_
 
 #include <stdint.h>
 
 #include <map>
@@ skipping 155 matching lines @@
   };
 
   // Queue of decoded frames in presentation order.
   std::priority_queue<linked_ptr<Frame>,
                       std::vector<linked_ptr<Frame>>,
                       FrameOrder> reorder_queue_;
 
   // Size of assigned picture buffers.
   gfx::Size picture_size_;
 
-  // Queue of frames that have not yet been decoded; maintains ownership of the
-  // Frame objects while they flow through VideoToolbox.
-  std::queue<linked_ptr<Frame>> pending_frames_;
+  // Frames that have not yet been decoded; maintains ownership of Frame objects
+  // while they flow through VideoToolbox.
+  std::map<Frame*, linked_ptr<Frame>> pending_frames_;

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

Why not just an std::set?

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

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]

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]

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]

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]

Sorry, that's DecodeTask().

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

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.

 
   // Set of assigned bitstream IDs, so that Destroy() can release them all.
   std::set<int32_t> assigned_bitstream_ids_;
 
   // All picture buffers assigned to us. Used to check if reused picture buffers
   // should be added back to the available list or released. (They are not
   // released immediately because we need the reuse event to free the binding.)
   std::set<int32_t> assigned_picture_ids_;
 
   // Texture IDs of assigned pictures.
@@ skipping 32 matching lines @@
   // Declared last to ensure that all weak pointers are invalidated before
   // other destructors run.
   base::WeakPtrFactory<VTVideoDecodeAccelerator> weak_this_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(VTVideoDecodeAccelerator);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_COMMON_GPU_MEDIA_VT_VIDEO_DECODE_ACCELERATOR_H_