Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
706023004:
Collect VTVideoDecodeAccelerator frames into a work queue (Closed)
|
Created:
6 years, 1 month ago by sandersd (OOO until July 31) Modified:
6 years, 1 month ago 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_config_change Project:
chromium Visibility:
Public. |
DescriptionCollect frames into a work queue to simplify the state machine. This
corrects a bug with flushing and will make it easier to implement the
reorder queue.
BUG=133828
Committed: https://crrev.com/9f5942b3e57bb17b1585bc80792c262044bc23b9
Cr-Commit-Position: refs/heads/master@{#305098}
Patch Set 1 #
Total comments: 32
Patch Set 2 : Address comments. #Patch Set 3 : Rebase. #
Total comments: 5
Patch Set 4 : Switch to ThreadChecker. #
Total comments: 10
Patch Set 5 : Address comments. #Patch Set 6 : Actually change the correct constant. #Patch Set 7 : Remove calls to weak_this_factory_. #Patch Set 8 : Rebase. #Patch Set 9 : Work around PPAPI test failure. #
Messages
Total messages: 29 (9 generated)
sandersd@chromium.org changed reviewers: + dalecurtis@chromium.org
Ignore gpu_video_decoder.cc, it's just a hack to force YouTube go through the H.264 path. It will be removed before submitting.
Whoops sorry, forgot to hit submit on my first round of comments. Still digging through this. You'll need have posciak@ or someone with VDA experience look over this too. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:139: std::vector<const uint8_t*> nalu_data_ptrs; Yuck, I guess you could const initialize parallel arrays directly, but it's of questionable cleanliness. I.e. const uint8_t* nalu_data_ptrs[] = { last_sps_.front(), last_spsext_.empty() ? last_pps_.front() : last_spsext_.front(), last_spsext_.empty() ? NULL : last_pps_.front() } At least reserve size 3 for each to avoid unnecessary growth? https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:166: // Flush all frames using the previous configuration to keep things simple. Shouldn't this happen in the Reset() that precedes a new ConfigureDecoder() call? https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:247: // NALUs are stored with Annex B format in the bitstream buffer (start codes), Peanut gallery: Seems like this entire NALU conversion process could be extracted into a helper function? Is this code necessary because FFmpegDemuxer invokes annex-b bitstream conversion? Should we just be disabling that for OSX instead? https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:302: if (config_changed) { Do you do this here instead of Initialize() since you need the sps/pps stuff? https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:304: LOG(ERROR) << "Invalid configuration data"; You should convert all these LOG(ERROR) calls to DLOG(ERROR) once this is complete. Most users won't see these which ends up just being binary bloat. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:641: // If this is a new flush reuqest, see if we can make progress. sp: request
https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:177: VTDecompressionSessionCanAcceptFormatDescription(session_, format_)) Multiline if should keep {} https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:333: data_size, // block_length How big is this typically? You may want to mark this with a TODO() to consider using a pool of memory blocks instead of recreating them for every frame. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:425: if (!CalledOnValidThread()) { If Output() is always going to call on the wrong thread, it seems it'd be better to always do the post there and then DCHECK here(). https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:451: DCHECK(CalledOnValidThread()); CalledOnValidThread isn't very readable when there are multiple threads :) Can you rename this to to indicate which thread is the valid thread? https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:460: Frame* frame = new Frame(bitstream.id()); Why does Frame have a bitstream_id instead of owning the bitstream buffer? https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:556: DCHECK_EQ(state_, STATE_NORMAL); This needs a comment on exactly what's happening here. The resetting logic seems backwards at first glance. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.h:145: std::queue<linked_ptr<Frame>> pending_frames_; Why queue vs vector ==> ScopedVector instead of linked_ptr?
https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:139: std::vector<const uint8_t*> nalu_data_ptrs; On 2014/11/10 20:48:02, DaleCurtis wrote: > Yuck, I guess you could const initialize parallel arrays directly, but it's of > questionable cleanliness. I.e. > > const uint8_t* nalu_data_ptrs[] = { > last_sps_.front(), > last_spsext_.empty() ? last_pps_.front() : last_spsext_.front(), > last_spsext_.empty() ? NULL : last_pps_.front() > } > > At least reserve size 3 for each to avoid unnecessary growth? Done. I'll see about a refactor to remove unnecessary heap allocations later. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:166: // Flush all frames using the previous configuration to keep things simple. On 2014/11/10 20:48:02, DaleCurtis wrote: > Shouldn't this happen in the Reset() that precedes a new ConfigureDecoder() > call? The decoder does not need to be reset to get a new configuration. (Although it also turns out that if I shuffle around when metadata is stored, I don't need this flush anyway.) https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:177: VTDecompressionSessionCanAcceptFormatDescription(session_, format_)) On 2014/11/10 21:10:39, DaleCurtis wrote: > Multiline if should keep {} Done. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:247: // NALUs are stored with Annex B format in the bitstream buffer (start codes), On 2014/11/10 20:48:02, DaleCurtis wrote: > Peanut gallery: Seems like this entire NALU conversion process could be > extracted into a helper function? That's probably true, but there are a lot of outputs. I'll see if I can find a clean way to do that in a future refactor. > Is this code necessary because FFmpegDemuxer invokes annex-b bitstream > conversion? Should we just be disabling that for OSX instead? Much of it is, but I would need to extract the SPS/PPS data either way (and for reordering, the slice headers too), and that is a lot easier using the H264Parser. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:302: if (config_changed) { On 2014/11/10 20:48:02, DaleCurtis wrote: > Do you do this here instead of Initialize() since you need the sps/pps stuff? That's part of it, but also the config can change at any frame. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:304: LOG(ERROR) << "Invalid configuration data"; On 2014/11/10 20:48:02, DaleCurtis wrote: > You should convert all these LOG(ERROR) calls to DLOG(ERROR) once this is > complete. Most users won't see these which ends up just being binary bloat. Ah, you're quite right, the "VDA Error" message printed by GPUVideoDecoder is also a DLOG(ERROR). This is going to make debugging failures in the wild a lot harder. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:333: data_size, // block_length On 2014/11/10 21:10:39, DaleCurtis wrote: > How big is this typically? You may want to mark this with a TODO() to consider > using a pool of memory blocks instead of recreating them for every frame. Done. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:425: if (!CalledOnValidThread()) { On 2014/11/10 21:10:39, DaleCurtis wrote: > If Output() is always going to call on the wrong thread, it seems it'd be better > to always do the post there and then DCHECK here(). Done. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:451: DCHECK(CalledOnValidThread()); On 2014/11/10 21:10:39, DaleCurtis wrote: > CalledOnValidThread isn't very readable when there are multiple threads :) Can > you rename this to to indicate which thread is the valid thread? I could add a wrapper method, but CalledOnValidThread() is a member of base:NonThreadSafe, and always means the thread the object was constructed on. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:460: Frame* frame = new Frame(bitstream.id()); On 2014/11/10 21:10:39, DaleCurtis wrote: > Why does Frame have a bitstream_id instead of owning the bitstream buffer? The ownership and lifetime story was not very clear, as the buffer becomes invalid once you call NotifyEndOfBitstreamBuffer() but the decoded frame stays valid (this isn't a big difference right now, but the reorder CL will to that before the frames enter the reorder buffer). In general I also wasn't happy with having a reference as a struct member. (There were stronger technical reasons before but they have been eliminated, so it's at least feasible to change now.) https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:556: DCHECK_EQ(state_, STATE_NORMAL); On 2014/11/10 21:10:39, DaleCurtis wrote: > This needs a comment on exactly what's happening here. The resetting logic seems > backwards at first glance. Done. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:641: // If this is a new flush reuqest, see if we can make progress. On 2014/11/10 20:48:02, DaleCurtis wrote: > sp: request Done. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.h:145: std::queue<linked_ptr<Frame>> pending_frames_; On 2014/11/10 21:10:39, DaleCurtis wrote: > Why queue vs vector ==> ScopedVector instead of linked_ptr? The short answer is that it makes moving ownership from this queue to |pending_tasks_| simpler. With a scoped_ptr, the task queue items would also need to be wrapped in smart pointers, so not only are the queue operations inefficient, there is also a lot of unnecessary boxing. That said I'm not extremely happy with the ownership story either way, the dance to pop items of this queue is not a good sign.
https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:304: LOG(ERROR) << "Invalid configuration data"; On 2014/11/11 18:46:07, sandersd wrote: > On 2014/11/10 20:48:02, DaleCurtis wrote: > > You should convert all these LOG(ERROR) calls to DLOG(ERROR) once this is > > complete. Most users won't see these which ends up just being binary bloat. > > Ah, you're quite right, the "VDA Error" message printed by GPUVideoDecoder is > also a DLOG(ERROR). This is going to make debugging failures in the wild a lot > harder. You might file a bug to do this after a default on canary release then so you can triage quickly. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:451: DCHECK(CalledOnValidThread()); On 2014/11/11 18:46:06, sandersd wrote: > On 2014/11/10 21:10:39, DaleCurtis wrote: > > CalledOnValidThread isn't very readable when there are multiple threads :) Can > > you rename this to to indicate which thread is the valid thread? > > I could add a wrapper method, but CalledOnValidThread() is a member of > base:NonThreadSafe, and always means the thread the object was constructed on. Hmm instead of inheriting non-threadsafe you should use a base::ThreadChecker. It's recommended to do this when multiple inheritance might occur (as it does for your class). https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:460: Frame* frame = new Frame(bitstream.id()); On 2014/11/11 18:46:07, sandersd wrote: > On 2014/11/10 21:10:39, DaleCurtis wrote: > > Why does Frame have a bitstream_id instead of owning the bitstream buffer? > > The ownership and lifetime story was not very clear, as the buffer becomes > invalid once you call NotifyEndOfBitstreamBuffer() but the decoded frame stays > valid (this isn't a big difference right now, but the reorder CL will to that > before the frames enter the reorder buffer). > > In general I also wasn't happy with having a reference as a struct member. > > (There were stronger technical reasons before but they have been eliminated, so > it's at least feasible to change now.) I'd spend some time cleaning up the ownership model sooner rather than later if you have an idea on how. https://codereview.chromium.org/706023004/diff/40001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/706023004/diff/40001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:318: weak_this_factory_.GetWeakPtr(), frame)); I don't think it's legal to create this WeakPtr here, you need to bind it on the GPUTaskRunner instead; I.e. keep a weak_this_ around that you can use from any thread. https://codereview.chromium.org/706023004/diff/40001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:424: weak_this_factory_.GetWeakPtr(), frame)); Ditto.
https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:304: LOG(ERROR) << "Invalid configuration data"; On 2014/11/11 21:02:34, DaleCurtis wrote: > On 2014/11/11 18:46:07, sandersd wrote: > > On 2014/11/10 20:48:02, DaleCurtis wrote: > > > You should convert all these LOG(ERROR) calls to DLOG(ERROR) once this is > > > complete. Most users won't see these which ends up just being binary bloat. > > > > Ah, you're quite right, the "VDA Error" message printed by GPUVideoDecoder is > > also a DLOG(ERROR). This is going to make debugging failures in the wild a lot > > harder. > > You might file a bug to do this after a default on canary release then so you > can triage quickly. Perhaps it would be better to temporarily add a lot of media log items? https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:451: DCHECK(CalledOnValidThread()); On 2014/11/11 21:02:34, DaleCurtis wrote: > On 2014/11/11 18:46:06, sandersd wrote: > > On 2014/11/10 21:10:39, DaleCurtis wrote: > > > CalledOnValidThread isn't very readable when there are multiple threads :) > Can > > > you rename this to to indicate which thread is the valid thread? > > > > I could add a wrapper method, but CalledOnValidThread() is a member of > > base:NonThreadSafe, and always means the thread the object was constructed on. > > > Hmm instead of inheriting non-threadsafe you should use a base::ThreadChecker. > It's recommended to do this when multiple inheritance might occur (as it does > for your class). Done. https://codereview.chromium.org/706023004/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:460: Frame* frame = new Frame(bitstream.id()); On 2014/11/11 21:02:34, DaleCurtis wrote: > On 2014/11/11 18:46:07, sandersd wrote: > > On 2014/11/10 21:10:39, DaleCurtis wrote: > > > Why does Frame have a bitstream_id instead of owning the bitstream buffer? > > > > The ownership and lifetime story was not very clear, as the buffer becomes > > invalid once you call NotifyEndOfBitstreamBuffer() but the decoded frame stays > > valid (this isn't a big difference right now, but the reorder CL will to that > > before the frames enter the reorder buffer). > > > > In general I also wasn't happy with having a reference as a struct member. > > > > (There were stronger technical reasons before but they have been eliminated, > so > > it's at least feasible to change now.) > > I'd spend some time cleaning up the ownership model sooner rather than later if > you have an idea on how. I do not, and this is the best I had on my third rewrite. The key problem is that the main thread needs to maintain ownership so that it can clean up properly, but in a logical sense the decoder owns the data while it is being decoded. https://codereview.chromium.org/706023004/diff/40001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/706023004/diff/40001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:318: weak_this_factory_.GetWeakPtr(), frame)); On 2014/11/11 21:02:34, DaleCurtis wrote: > I don't think it's legal to create this WeakPtr here, you need to bind it on the > GPUTaskRunner instead; I.e. keep a weak_this_ around that you can use from any > thread. As far as I can tell, there is no meaningful difference between copying a weak pointer and creating a new one. It is critical that all weak pointers are *dereferenced* on the same thread, but that is guaranteed as we only use them for posting tasks. https://codereview.chromium.org/706023004/diff/40001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:424: weak_this_factory_.GetWeakPtr(), frame)); On 2014/11/11 21:02:34, DaleCurtis wrote: > Ditto. Acknowledged.
lgtm, but again, you'll need someone with more VDA experience for final approval. https://codereview.chromium.org/706023004/diff/40001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/706023004/diff/40001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:318: weak_this_factory_.GetWeakPtr(), frame)); On 2014/11/11 22:14:39, sandersd wrote: > On 2014/11/11 21:02:34, DaleCurtis wrote: > > I don't think it's legal to create this WeakPtr here, you need to bind it on > the > > GPUTaskRunner instead; I.e. keep a weak_this_ around that you can use from any > > thread. > > As far as I can tell, there is no meaningful difference between copying a weak > pointer and creating a new one. > > It is critical that all weak pointers are *dereferenced* on the same thread, but > that is guaranteed as we only use them for posting tasks. There's a difference if you're calling InvalidateWeakPtrs(), since you would then be copying a nulled WeakPtr, but you're not in this class. Yeah it looks like you're correct, I could have sworn I remember you couldn't create them on different threads either, but I don't see that in the code.
Oh, the bug number is messed up above too.
sandersd@chromium.org changed reviewers: + posciak@chromium.org
posciak: please take a look for VDA API.
I'm really liking the changes. https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:38: static const int kNumPictureBuffers = 5; Would we want to use media::limits::kMaxVideoFrames here instead? https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:233: Frame* frame) { I would strongly prefer if you used and passed scopers around. I understand that you are careful to ensure the lifetime is right, but there should be no need to worry about this now and in the future. https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:512: break; Does this mean we loop here forever, since pending_tasks_ never becomes empty and Destroy cannot execute, since we are blocking gpu thread? https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:59: STATE_NORMAL, Maybe DECODING? RUNNING?
https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:38: static const int kNumPictureBuffers = 5; On 2014/11/12 12:57:30, Pawel Osciak wrote: > Would we want to use media::limits::kMaxVideoFrames here instead? media::limits::kMaxVideoFrames + 1 seems to be a popular choice, the best I can say for sure is that it should not be smaller than media::limits::kMaxVideoFrames. (Done.) https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:233: Frame* frame) { On 2014/11/12 12:57:30, Pawel Osciak wrote: > I would strongly prefer if you used and passed scopers around. I understand that > you are careful to ensure the lifetime is right, but there should be no need to > worry about this now and in the future. I'm currently considering two options here: 1) Create a new map to own the frame objects (the same way the that |assigned_bitstream_ids_| works, but with a longer life), and then always use raw pointers for frames elsewhere. This simplifies the definition of the queues a lot, and one place to look to understand the ownership rules. 2) Use scoped_ptr to handle ownership of the Frame objects everywhere. This means we have to trust the VT API a little more, and will need to wrap queue items in linked_ptrs, but in each individual location it will be easy to explain why ownership is handled the way it is (except maybe at destruction, which may be trickier, not sure). I can try both, but I'd like to finish the reorder and sandbox CLs first. https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:512: break; On 2014/11/12 12:57:30, Pawel Osciak wrote: > Does this mean we loop here forever, since pending_tasks_ never becomes empty > and Destroy cannot execute, since we are blocking gpu thread? Done. https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:59: STATE_NORMAL, On 2014/11/12 12:57:30, Pawel Osciak wrote: > Maybe DECODING? RUNNING? Done.
sandersd@chromium.org changed required reviewers: + dalecurtis@chromium.org, posciak@chromium.org
posciak: ping?
lgtm https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:38: static const int kNumPictureBuffers = 5; On 2014/11/13 00:05:45, sandersd wrote: > On 2014/11/12 12:57:30, Pawel Osciak wrote: > > Would we want to use media::limits::kMaxVideoFrames here instead? > > media::limits::kMaxVideoFrames + 1 seems to be a popular choice Yes, at least +1 is what I meant, thanks. The reason for this is that the pipeline needs that much plus at least one for us to work on. https://chromiumcodereview.appspot.com/706023004/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:233: Frame* frame) { On 2014/11/13 00:05:45, sandersd wrote: > On 2014/11/12 12:57:30, Pawel Osciak wrote: > > I would strongly prefer if you used and passed scopers around. I understand > that > > you are careful to ensure the lifetime is right, but there should be no need > to > > worry about this now and in the future. > > I'm currently considering two options here: > 1) Create a new map to own the frame objects (the same way the that > |assigned_bitstream_ids_| works, but with a longer life), and then always use > raw pointers for frames elsewhere. This simplifies the definition of the queues > a lot, and one place to look to understand the ownership rules. > 2) Use scoped_ptr to handle ownership of the Frame objects everywhere. This > means we have to trust the VT API a little more, and will need to wrap queue > items in linked_ptrs, but in each individual location it will be easy to explain > why ownership is handled the way it is (except maybe at destruction, which may > be trickier, not sure). > > I can try both, but I'd like to finish the reorder and sandbox CLs first. How about 2), but when the frame is in decoding, put the scoper on a frames_in_decode_ list, and in case something goes wrong only clean it up after decoder thread is stopped to ensure VT can't be running anymore?
Patchset #7 (id:120001) 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/706023004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/706023004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/706023004/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9f5942b3e57bb17b1585bc80792c262044bc23b9 Cr-Commit-Position: refs/heads/master@{#305098} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
