Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
491163002:
Implement flushing in VTVideoDecodeAccelerator. (Closed)
|
Created:
6 years, 4 months ago by sandersd (OOO until July 31) Modified:
6 years, 2 months ago Reviewers:
Pawel Osciak 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@master Project:
chromium Visibility:
Public. |
DescriptionImplement flushing in VTVideoDecodeAccelerator.
BUG=133828
Committed: https://crrev.com/063dbc21c6cef08fcfec2bce1290d76358a323ba
Cr-Commit-Position: refs/heads/master@{#296980}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase. #Patch Set 3 : Rebase #Patch Set 4 : New state machine. #
Total comments: 67
Patch Set 5 : Address comments. #
Total comments: 4
Patch Set 6 : Fix comment. #Patch Set 7 : Reorganize ProcessDecodedFrames(). #Patch Set 8 : Correct QueueAction(ACTION_DESTROY). #
Total comments: 4
Patch Set 9 : Move CompleteAction() into case ACTION_DESTROY. #
Messages
Total messages: 19 (1 generated)
https://codereview.chromium.org/491163002/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/491163002/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:185: frames_pending_decode_++; Client will keep calling Decode()s after it calls Reset(), and before it gets NotifyResetDone(), for the frames to be decoded after Reset(). So this is not precise I think, or may not even work at all, because this may never drop down to 0... Am I missing something?
https://codereview.chromium.org/491163002/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/491163002/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:185: frames_pending_decode_++; On 2014/08/27 08:25:20, Pawel Osciak wrote: > Client will keep calling Decode()s after it calls Reset(), and before it gets > NotifyResetDone(), for the frames to be decoded after Reset(). So this is not > precise I think, or may not even work at all, because this may never drop down > to 0... Am I missing something? video_decoder.h specifies that no decode calls are made during a reset: // Note: No VideoDecoder calls should be made before |closure| is executed. And as far as I can tell, DecoderStream correctly avoids making any Decode() calls while a reset is happening.
https://codereview.chromium.org/491163002/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/491163002/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:185: frames_pending_decode_++; On 2014/08/27 21:40:43, sandersd wrote: > On 2014/08/27 08:25:20, Pawel Osciak wrote: > > Client will keep calling Decode()s after it calls Reset(), and before it gets > > NotifyResetDone(), for the frames to be decoded after Reset(). So this is not > > precise I think, or may not even work at all, because this may never drop down > > to 0... Am I missing something? > > video_decoder.h specifies that no decode calls are made during a reset: > > // Note: No VideoDecoder calls should be made before |closure| is executed. > > And as far as I can tell, DecoderStream correctly avoids making any Decode() > calls while a reset is happening. This class implements VideoDecodeAccelerator, not VideoDecoder :) GpuVideoDecoder, the VideoDecoder implementation that is the HTML5 <video> client of VDA impls, is a VD, but other VDA clients are not. In fact, one significant example is Flash, the ppapi-based client of VDA, which does call Decode immediately after Reset, before getting NotifyResetDone. We had to address this once before, see crrev.com/179384 for example. But this is fully intended behavior, so it was just a bug in one of the VDAs that we had to fix. And, well, it's slightly more efficient to accept Decode while resetting and there is no real reason not to. I think I'll file an exploratory bug why we can't accumulate more buffers from VideoDecoder. You could solve this perhaps by managing Reset like other VDAs, see VaapiVDA for example.
> This class implements VideoDecodeAccelerator, not VideoDecoder :) > GpuVideoDecoder, the VideoDecoder implementation that is the HTML5 <video> > client of VDA impls, is a VD, but other VDA clients are not. Sure, but GpuVideoDecoder is the only implementation I can examine, and the VDA header file is rather ambiguous. Specifically: // Resets the decoder: all pending inputs are dropped immediately and the // decoder returned to a state ready for further Decode()s, followed by // NotifyResetDone() being called on the client. Can be used to implement // "seek". Can be interpreted this to mean that the decoder is not ready for further Decode()s until it calls NotifyResetDone(). Do I at least get to assume that Flush() and Reset() can't be overlapped with themselves or each other? Are Decode() requests allowed during Flush() operations? > In fact, one significant example is Flash, the ppapi-based client of VDA, which > does call Decode immediately after Reset, before getting NotifyResetDone. We had > to address this once before, see crrev.com/179384 for example. But this is fully > intended behavior, so it was just a bug in one of the VDAs that we had to fix. > > And, well, it's slightly more efficient to accept Decode while resetting and > there is no real reason not to. I think I'll file an exploratory bug why we > can't accumulate more buffers from VideoDecoder. > > You could solve this perhaps by managing Reset like other VDAs, see VaapiVDA for > example. Vaapi has this to say: // All the decoding tasks from before the reset request from client are done // by now, as this task was scheduled after them and client is expected not // to call Decode() after Reset() and before NotifyResetDone. The main problem we have for VT is that there is no Reset(), only Flush(). Whatever the specific requirements are, I'll need to develop a new way to handle it.
On 2014/08/28 18:12:56, sandersd wrote: > > This class implements VideoDecodeAccelerator, not VideoDecoder :) > > GpuVideoDecoder, the VideoDecoder implementation that is the HTML5 <video> > > client of VDA impls, is a VD, but other VDA clients are not. > > Sure, but GpuVideoDecoder is the only implementation I can examine, and the VDA > header file is rather ambiguous. Specifically: > > // Resets the decoder: all pending inputs are dropped immediately and the > // decoder returned to a state ready for further Decode()s, followed by > // NotifyResetDone() being called on the client. Can be used to implement > // "seek". > > Can be interpreted this to mean that the decoder is not ready for further > Decode()s until it calls NotifyResetDone(). I guess the comment could be improved. But it says first we drop, then the decoder is returned to a state ready for further Decode()s, and then that is followed by NotifyResetDone(). So in that order, i.e. Decode()s can come before NRD(). > Do I at least get to assume that Flush() and Reset() can't be overlapped with > themselves or each other? If you handle them separately, it shouldn't hopefully be an issue to handle this. Just handle them in order as posted. 1) Flush -> Reset Finish all Decode()s from before Flush, return all output buffers. Reset() then does nothing, apart from putting you in a state after reset, which requires a resume point and should drop any Decode() calls that are not a resume point (and not produce corruption/errors instead). 2) Reset -> Flush Do reset, then Flush handler just makes sure there is nothing pending and all outputs are returned. I think if you handle things by posting tasks in order, you should be able to handle them in order. > Are Decode() requests allowed during Flush() operations? No. > > In fact, one significant example is Flash, the ppapi-based client of VDA, > which > > does call Decode immediately after Reset, before getting NotifyResetDone. We > had > > to address this once before, see crrev.com/179384 for example. But this is > fully > > intended behavior, so it was just a bug in one of the VDAs that we had to fix. > > > > And, well, it's slightly more efficient to accept Decode while resetting and > > there is no real reason not to. I think I'll file an exploratory bug why we > > can't accumulate more buffers from VideoDecoder. > > > > You could solve this perhaps by managing Reset like other VDAs, see VaapiVDA > for > > example. > > Vaapi has this to say: > > // All the decoding tasks from before the reset request from client are done > // by now, as this task was scheduled after them and client is expected not > // to call Decode() after Reset() and before NotifyResetDone. > Sorry, this is an outdated comment from before the fix I had mentioned (crrev.com/179384). > The main problem we have for VT is that there is no Reset(), only Flush(). > Whatever the specific requirements are, I'll need to develop a new way to handle > it. What happens if we call reset and then call Decode() with frames that are not good resume points? Corruption? Errors?
A new state machine for you! > I think if you handle things by posting tasks in order, you should be able to > handle them in order. Because decoded frames (including frames during a reset operation) are posted to the main thread, this doesn't quite work. But the new state machine queues them up, so it should be the same in the end. > What happens if we call reset and then call Decode() with frames that are not > good resume points? Corruption? Errors? A good question, I'll give it a try. Hopefully VT will just drop the frame if it doesn't have the necessary reference frames, which would result in NotifyEndOfBitstreamBuffer() but no PictureReady(). If not, the only real option is to destroy the VTDecompressionSession on Reset().
https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:248: if (!data_size) { Could you explain? If there is no data we want to output? https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:386: pending_actions_.front().bitstream_id == bitstream_id) { Wouldn't you want to do <= instead of == here? https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:395: while (true) { Maybe just while (!decoded_frames_.empty()) instead? https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:404: } else if (pending_actions_.front().action == ACTION_FLUSH) { I think it could be more readable if you: if (pending_actions_.empty()) { ... return; } switch (pending_actions_.front().action) { case ACTION_FLUSH: case ACTION_RESET: case ACTION_DESTROY: ... } https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:406: if (!available_picture_ids_.empty()) { You make this check in SendPictures anyway. I'd remove it from here and above. You return -1 from SendPictures if it was empty anyway. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:413: return; I'd just remove continue and return and let the existing conditions above take care of this to make this more readable. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:415: // Reseting; drop a decoded frame and then complete any actions. One frame? Not drop frames until the id for the action? https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:416: int32_t bitstream_id = decoded_frames_.front().bitstream_id; Why not do things until action's bitstream id? This feels like an unnecessary dependency on the relationship between the action and pending and decoded frames... I think it's better to make it clear and simple, drop all until id in action. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:424: continue; Not needed. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:426: // Destroying; drop a decoded frame, then destroy if ready. Why only one not up to action id? That would be simpler and more readable without having to make the assumption there was only one. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:438: NOTREACHED(); This would go to default: clause. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:460: if (image_buffer) { What happens if there is no image_buffer? Don't we have to do something with the Picture anyway? Or will we just hang? I think this is a NotifyError() situation... https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:463: // TODO(sandersd): Find out why this somtimes fails due to no GL context. s/somtimes/sometimes/ How do you know it failed? How often does this happen? Was it happening before this change? If you know it failed, NotifyError(). https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:484: if (frame.bitstream_id == up_to_bitstream_id) Make this a part of while() condition? https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:500: CompleteAction(action); Could you just let ProcessDecodedFrames() to this to have one place that handles this case? https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:506: &VTVideoDecodeAccelerator::FlushTask, base::Unretained(this))); This requires an explanation... https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:522: DCHECK(CalledOnValidThread()); If you stopped the decoder thread here, you wouldn't have to ensure it was defined last in the class. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:528: client_->NotifyEndOfBitstreamBuffer(pending_bitstream_ids_.front()); You shouldn't have to do this on Destroy(). https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:531: QueueAction(ACTION_DESTROY); Do we actually need this? You dropped everything already above anyway. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:67: enum Action { Please document this and the class. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:96: void ProcessDecodedFrames(); Please document.
https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:248: if (!data_size) { On 2014/09/25 01:27:29, Pawel Osciak wrote: > Could you explain? If there is no data we want to output? Done. (And yes.) https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:386: pending_actions_.front().bitstream_id == bitstream_id) { On 2014/09/25 01:27:29, Pawel Osciak wrote: > Wouldn't you want to do <= instead of == here? No, CompleteActions() is called every time a pending action is due, it can't miss any. Its only job is to call *all* the due actions for a specific bitstream_id. I suspect that there are no cases where there is more than one, but I couldn't prove it can't happen based on our assumptions. In general, it's an error to assume bitstream IDs are ordered (in fact they wrap, but admittedly only every 200 days for 60fps content). https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:395: while (true) { On 2014/09/25 01:27:29, Pawel Osciak wrote: > Maybe just while (!decoded_frames_.empty()) instead? Done. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:404: } else if (pending_actions_.front().action == ACTION_FLUSH) { On 2014/09/25 01:27:28, Pawel Osciak wrote: > I think it could be more readable if you: > > if (pending_actions_.empty()) { > ... > return; > } > > switch (pending_actions_.front().action) { > case ACTION_FLUSH: > case ACTION_RESET: > case ACTION_DESTROY: > ... > } I like this, thanks! https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:406: if (!available_picture_ids_.empty()) { On 2014/09/25 01:27:28, Pawel Osciak wrote: > You make this check in SendPictures anyway. I'd remove it from here and above. > You return -1 from SendPictures if it was empty anyway. This check served a few purposes: - It avoids needlessly setting up GL context (which is the same as adding an early exit in SendPictures()). - It avoids passing -1 to CompleteActions(). I was avoiding this because bitstream_ids are not actually spec'd to be non-negative. - It distinguishes whether to return or loop again, as if the queue is non-empty SendPictures() is guaranteed to send a frame (or enter an error state, but that's not implemented yet). Under the assumption you believe that using negative bitstream_ids as flags is safe, I've added a negativity CHECK for bitstream_ids, and I'll start using that optimization where possible. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:413: return; On 2014/09/25 01:27:29, Pawel Osciak wrote: > I'd just remove continue and return and let the existing conditions above take > care of this to make this more readable. Acknowledged. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:415: // Reseting; drop a decoded frame and then complete any actions. On 2014/09/25 01:27:28, Pawel Osciak wrote: > One frame? Not drop frames until the id for the action? It's the same, but I've switched it around. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:416: int32_t bitstream_id = decoded_frames_.front().bitstream_id; On 2014/09/25 01:27:28, Pawel Osciak wrote: > Why not do things until action's bitstream id? This feels like an unnecessary > dependency on the relationship between the action and pending and decoded > frames... > > I think it's better to make it clear and simple, drop all until id in action. Done. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:424: continue; On 2014/09/25 01:27:29, Pawel Osciak wrote: > Not needed. Acknowledged. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:426: // Destroying; drop a decoded frame, then destroy if ready. On 2014/09/25 01:27:28, Pawel Osciak wrote: > Why only one not up to action id? That would be simpler and more readable > without having to make the assumption there was only one. Done. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:438: NOTREACHED(); On 2014/09/25 01:27:29, Pawel Osciak wrote: > This would go to default: clause. Not quite, I want to be sure that every path at the end of a block is explicit (return or continue), and this does that. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:460: if (image_buffer) { On 2014/09/25 01:27:28, Pawel Osciak wrote: > What happens if there is no image_buffer? Don't we have to do something with the > Picture anyway? Or will we just hang? I think this is a NotifyError() > situation... This happens in if !data_size, or if the decoder decides to drop a frame (which I assume happens if the reference frames are missing, but I've not tested yet). We skip sending the picture but still notify the end of the bitstream buffer. I don't know a correct behavior for this case, but I can reproduce getting a bitstream buffer with no image data in it (only stream configuration), so we'll need to handle it. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:463: // TODO(sandersd): Find out why this somtimes fails due to no GL context. On 2014/09/25 01:27:28, Pawel Osciak wrote: > s/somtimes/sometimes/ > > How do you know it failed? How often does this happen? Was it happening before > this change? > If you know it failed, NotifyError(). It fails when the CGLTexImageIOSurface2D() check fails. It happened before, but it's flaky and I can't reliably reproduce it. I intend to find a CGL expert to help me with this one. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:484: if (frame.bitstream_id == up_to_bitstream_id) On 2014/09/25 01:27:29, Pawel Osciak wrote: > Make this a part of while() condition? Done. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:500: CompleteAction(action); On 2014/09/25 01:27:29, Pawel Osciak wrote: > Could you just let ProcessDecodedFrames() to this to have one place that handles > this case? ProcessDecodedFrames() doesn't currently handle any cases where there are no queued decoded frames. It would need a rename if we moved this logic there, and I didn't find any good names for it! If you really prefer that, I'd be happy to do a followup CL for it. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:506: &VTVideoDecodeAccelerator::FlushTask, base::Unretained(this))); On 2014/09/25 01:27:28, Pawel Osciak wrote: > This requires an explanation... I think the condition was just wrong, as there will never be pending actions if there are no pending frames. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:522: DCHECK(CalledOnValidThread()); On 2014/09/25 01:27:29, Pawel Osciak wrote: > If you stopped the decoder thread here, you wouldn't have to ensure it was > defined last in the class. This is indeed possible, as long as the FlushTask is queued first, but it duplicates the implementation of QueueAction(). I'd like to think about it for a while as a possible future refactoring. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:528: client_->NotifyEndOfBitstreamBuffer(pending_bitstream_ids_.front()); On 2014/09/25 01:27:29, Pawel Osciak wrote: > You shouldn't have to do this on Destroy(). It seems I do, GpuVideoDecoder DCHECKS that there are no buffers still in the decoder in ~GpuVideoDecoder(). https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:531: QueueAction(ACTION_DESTROY); On 2014/09/25 01:27:29, Pawel Osciak wrote: > Do we actually need this? You dropped everything already above anyway. We tell the client we are done with the buffers, but VT may still have to flush (and while it's doing that, it will be calling back with a reference to |this|). It may be possible to destroy the VT session and avoid that, but I'll need to ask the Apple engineers if that is guaranteed to work (fits with the stopping the decoder thread refactor from above). https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:67: enum Action { On 2014/09/25 01:27:29, Pawel Osciak wrote: > Please document this and the class. Done. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:96: void ProcessDecodedFrames(); On 2014/09/25 01:27:29, Pawel Osciak wrote: > Please document. Done.
https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:386: pending_actions_.front().bitstream_id == bitstream_id) { On 2014/09/25 19:18:53, sandersd wrote: > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > Wouldn't you want to do <= instead of == here? > > No, CompleteActions() is called every time a pending action is due, it can't > miss any. Its only job is to call *all* the due actions for a specific > bitstream_id. I suspect that there are no cases where there is more than one, > but I couldn't prove it can't happen based on our assumptions. > > In general, it's an error to assume bitstream IDs are ordered (in fact they > wrap, but admittedly only every 200 days for 60fps content). Ah, lack of doc for the method confused me. And I like your thinking about the wraparounds. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:413: return; On 2014/09/25 19:18:53, sandersd wrote: > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > I'd just remove continue and return and let the existing conditions above take > > care of this to make this more readable. > > Acknowledged. But not implemented? https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:424: continue; On 2014/09/25 19:18:52, sandersd wrote: > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > Not needed. > > Acknowledged. But not removed? https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:438: NOTREACHED(); On 2014/09/25 19:18:52, sandersd wrote: > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > This would go to default: clause. > > Not quite, I want to be sure that every path at the end of a block is explicit > (return or continue), and this does that. You can do that by removing continues and putting NOTREACHED in default clause. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:463: // TODO(sandersd): Find out why this somtimes fails due to no GL context. On 2014/09/25 19:18:53, sandersd wrote: > On 2014/09/25 01:27:28, Pawel Osciak wrote: > > s/somtimes/sometimes/ > > > > How do you know it failed? How often does this happen? Was it happening before > > this change? > > If you know it failed, NotifyError(). > > It fails when the CGLTexImageIOSurface2D() check fails. It happened before, but > it's flaky and I can't reliably reproduce it. I intend to find a CGL expert to > help me with this one. Well, I'd really like to express my strong preference for NotifyError over crashing the process once again here FTR. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:522: DCHECK(CalledOnValidThread()); On 2014/09/25 19:18:52, sandersd wrote: > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > If you stopped the decoder thread here, you wouldn't have to ensure it was > > defined last in the class. > > This is indeed possible, as long as the FlushTask is queued first, but it > duplicates the implementation of QueueAction(). I'd like to think about it for a > while as a possible future refactoring. I don't understand the part about FlushTask... Queued first before what? https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:528: client_->NotifyEndOfBitstreamBuffer(pending_bitstream_ids_.front()); On 2014/09/25 19:18:53, sandersd wrote: > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > You shouldn't have to do this on Destroy(). > > It seems I do, GpuVideoDecoder DCHECKS that there are no buffers still in the > decoder in ~GpuVideoDecoder(). Acknowledged. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:531: QueueAction(ACTION_DESTROY); On 2014/09/25 19:18:52, sandersd wrote: > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > Do we actually need this? You dropped everything already above anyway. > > We tell the client we are done with the buffers, but VT may still have to flush > (and while it's doing that, it will be calling back with a reference to |this|). > It may be possible to destroy the VT session and avoid that, but I'll need to > ask the Apple engineers if that is guaranteed to work (fits with the stopping > the decoder thread refactor from above). How do you guarantee that during that client_ will not be called anymore? https://chromiumcodereview.appspot.com/491163002/diff/80001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/491163002/diff/80001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:519: // If there are still pending actions, then a flush may be required. I'm still not clear why a flush is required? If it's a flush action, it will flush, if any other, we shouldn't flush? https://chromiumcodereview.appspot.com/491163002/diff/80001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/491163002/diff/80001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:76: // sent, a particular |action| should be completed. s/completed/completed at/
https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:386: pending_actions_.front().bitstream_id == bitstream_id) { On 2014/09/26 04:25:07, Pawel Osciak wrote: > On 2014/09/25 19:18:53, sandersd wrote: > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > Wouldn't you want to do <= instead of == here? > > > > No, CompleteActions() is called every time a pending action is due, it can't > > miss any. Its only job is to call *all* the due actions for a specific > > bitstream_id. I suspect that there are no cases where there is more than one, > > but I couldn't prove it can't happen based on our assumptions. > > > > In general, it's an error to assume bitstream IDs are ordered (in fact they > > wrap, but admittedly only every 200 days for 60fps content). > > Ah, lack of doc for the method confused me. > And I like your thinking about the wraparounds. Acknowledged. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:413: return; On 2014/09/26 04:25:07, Pawel Osciak wrote: > On 2014/09/25 19:18:53, sandersd wrote: > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > I'd just remove continue and return and let the existing conditions above > take > > > care of this to make this more readable. > > > > Acknowledged. > > But not implemented? I thought the switch version made this clear enough. My worry here is that the Flush case makes no progress if there are no pictures available, and I'd need to detect that specific case to abort, resulting in the same branch either way. Similarly, DESTROY must return (it would be an error to evaluate the loop conditions). The only flow that can be changed is RESET, but I decided to favor consistency. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:424: continue; On 2014/09/26 04:25:07, Pawel Osciak wrote: > On 2014/09/25 19:18:52, sandersd wrote: > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > Not needed. > > > > Acknowledged. > > But not removed? See above. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:438: NOTREACHED(); On 2014/09/26 04:25:07, Pawel Osciak wrote: > On 2014/09/25 19:18:52, sandersd wrote: > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > This would go to default: clause. > > > > Not quite, I want to be sure that every path at the end of a block is explicit > > (return or continue), and this does that. > > You can do that by removing continues and putting NOTREACHED in default clause. I don't believe that is the same. The compiler can tell me if I am missing a case in the switch, and the continues are inside if blocks , changing them to 'break' is just more confusing. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:463: // TODO(sandersd): Find out why this somtimes fails due to no GL context. On 2014/09/26 04:25:07, Pawel Osciak wrote: > On 2014/09/25 19:18:53, sandersd wrote: > > On 2014/09/25 01:27:28, Pawel Osciak wrote: > > > s/somtimes/sometimes/ > > > > > > How do you know it failed? How often does this happen? Was it happening > before > > > this change? > > > If you know it failed, NotifyError(). > > > > It fails when the CGLTexImageIOSurface2D() check fails. It happened before, > but > > it's flaky and I can't reliably reproduce it. I intend to find a CGL expert to > > help me with this one. > > Well, I'd really like to express my strong preference for NotifyError over > crashing the process once again here FTR. Understood, replacing all the CHECKs is the next item on the TODO list. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:522: DCHECK(CalledOnValidThread()); On 2014/09/26 04:25:07, Pawel Osciak wrote: > On 2014/09/25 19:18:52, sandersd wrote: > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > If you stopped the decoder thread here, you wouldn't have to ensure it was > > > defined last in the class. > > > > This is indeed possible, as long as the FlushTask is queued first, but it > > duplicates the implementation of QueueAction(). I'd like to think about it for > a > > while as a possible future refactoring. > > I don't understand the part about FlushTask... Queued first before what? Queued to the decoder thread message loop, before shutting down the decoder thread (so that the flush method gets called on VT, which is only done from the decoder thread). https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:531: QueueAction(ACTION_DESTROY); On 2014/09/26 04:25:07, Pawel Osciak wrote: > On 2014/09/25 19:18:52, sandersd wrote: > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > Do we actually need this? You dropped everything already above anyway. > > > > We tell the client we are done with the buffers, but VT may still have to > flush > > (and while it's doing that, it will be calling back with a reference to > |this|). > > It may be possible to destroy the VT session and avoid that, but I'll need to > > ask the Apple engineers if that is guaranteed to work (fits with the stopping > > the decoder thread refactor from above). > > How do you guarantee that during that client_ will not be called anymore? During destroy, ProcessDecodedFrames() specifically does not call SendPictures() or NotifyEndOfBitstreamBuffer(), but instead drops all the frames without any bookkeeping. https://chromiumcodereview.appspot.com/491163002/diff/80001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/491163002/diff/80001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:519: // If there are still pending actions, then a flush may be required. On 2014/09/26 04:25:08, Pawel Osciak wrote: > I'm still not clear why a flush is required? If it's a flush action, it will > flush, if any other, we shouldn't flush? We need to flush for all three, because there is no way to reset the decoder. As spec'd, the decoder may delay returning frames indefinitely until it is flushed, in which case we will be waiting forever to complete the reset or destroy. (Note that the VT method is actually called VTDecompressionSessionFinishDelayedFrames.) https://chromiumcodereview.appspot.com/491163002/diff/80001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/491163002/diff/80001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:76: // sent, a particular |action| should be completed. On 2014/09/26 04:25:08, Pawel Osciak wrote: > s/completed/completed at/ Done.
https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:413: return; On 2014/09/26 04:56:41, sandersd wrote: > On 2014/09/26 04:25:07, Pawel Osciak wrote: > > On 2014/09/25 19:18:53, sandersd wrote: > > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > > I'd just remove continue and return and let the existing conditions above > > take > > > > care of this to make this more readable. > > > > > > Acknowledged. > > > > But not implemented? > > I thought the switch version made this clear enough. > > My worry here is that the Flush case makes no progress if there are no pictures > available, and I'd need to detect that specific case to abort, resulting in the > same branch either way. Similarly, DESTROY must return (it would be an error to > evaluate the loop conditions). The only flow that can be changed is RESET, but I > decided to favor consistency. My point is, continue is the default action. So you don't have to write it if you instead return as the non-default action of each case. That's how people are used to looking at switch()es, return is the non-default action, break is the default. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:438: NOTREACHED(); On 2014/09/26 04:56:41, sandersd wrote: > On 2014/09/26 04:25:07, Pawel Osciak wrote: > > On 2014/09/25 19:18:52, sandersd wrote: > > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > > This would go to default: clause. > > > > > > Not quite, I want to be sure that every path at the end of a block is > explicit > > > (return or continue), and this does that. > > > > You can do that by removing continues and putting NOTREACHED in default > clause. > > I don't believe that is the same. The compiler can tell me if I am missing a > case in the switch, and the continues are inside if blocks , changing them to > 'break' is just more confusing. That's exactly the point I'm trying to make. Nobody expects "return" to be default and "break" (de facto continue in this case) to be the non-default action, when looking at a switch(){} https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:522: DCHECK(CalledOnValidThread()); On 2014/09/26 04:56:41, sandersd wrote: > On 2014/09/26 04:25:07, Pawel Osciak wrote: > > On 2014/09/25 19:18:52, sandersd wrote: > > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > > If you stopped the decoder thread here, you wouldn't have to ensure it was > > > > defined last in the class. > > > > > > This is indeed possible, as long as the FlushTask is queued first, but it > > > duplicates the implementation of QueueAction(). I'd like to think about it > for > > a > > > while as a possible future refactoring. > > > > I don't understand the part about FlushTask... Queued first before what? > > Queued to the decoder thread message loop, before shutting down the decoder > thread (so that the flush method gets called on VT, which is only done from the > decoder thread). If you decoder_thread_.Stop() from Destroy(), it will wait until all tasks are done on decoder_thread_. If you require a FlushTask to always be done on Destroy, just queue it explicitly here and then Stop()? https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:531: QueueAction(ACTION_DESTROY); On 2014/09/26 04:56:41, sandersd wrote: > On 2014/09/26 04:25:07, Pawel Osciak wrote: > > On 2014/09/25 19:18:52, sandersd wrote: > > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > > Do we actually need this? You dropped everything already above anyway. > > > > > > We tell the client we are done with the buffers, but VT may still have to > > flush > > > (and while it's doing that, it will be calling back with a reference to > > |this|). > > > It may be possible to destroy the VT session and avoid that, but I'll need > to > > > ask the Apple engineers if that is guaranteed to work (fits with the > stopping > > > the decoder thread refactor from above). > > > > How do you guarantee that during that client_ will not be called anymore? > > During destroy, ProcessDecodedFrames() specifically does not call SendPictures() > or NotifyEndOfBitstreamBuffer(), but instead drops all the frames without any > bookkeeping. Could you tell me how this is achieved please?
https://codereview.chromium.org/491163002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/491163002/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:413: return; On 2014/09/26 05:20:30, Pawel Osciak wrote: > On 2014/09/26 04:56:41, sandersd wrote: > > On 2014/09/26 04:25:07, Pawel Osciak wrote: > > > On 2014/09/25 19:18:53, sandersd wrote: > > > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > > > I'd just remove continue and return and let the existing conditions > above > > > take > > > > > care of this to make this more readable. > > > > > > > > Acknowledged. > > > > > > But not implemented? > > > > I thought the switch version made this clear enough. > > > > My worry here is that the Flush case makes no progress if there are no > pictures > > available, and I'd need to detect that specific case to abort, resulting in > the > > same branch either way. Similarly, DESTROY must return (it would be an error > to > > evaluate the loop conditions). The only flow that can be changed is RESET, but > I > > decided to favor consistency. > > My point is, continue is the default action. So you don't have to write it if > you instead return as the non-default action of each case. That's how people are > used to looking at switch()es, return is the non-default action, break is the > default. Done. https://codereview.chromium.org/491163002/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:438: NOTREACHED(); On 2014/09/26 05:20:30, Pawel Osciak wrote: > On 2014/09/26 04:56:41, sandersd wrote: > > On 2014/09/26 04:25:07, Pawel Osciak wrote: > > > On 2014/09/25 19:18:52, sandersd wrote: > > > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > > > This would go to default: clause. > > > > > > > > Not quite, I want to be sure that every path at the end of a block is > > explicit > > > > (return or continue), and this does that. > > > > > > You can do that by removing continues and putting NOTREACHED in default > > clause. > > > > I don't believe that is the same. The compiler can tell me if I am missing a > > case in the switch, and the continues are inside if blocks , changing them to > > 'break' is just more confusing. > > That's exactly the point I'm trying to make. Nobody expects "return" to be > default and "break" (de facto continue in this case) to be the non-default > action, when looking at a switch(){} Done. https://codereview.chromium.org/491163002/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:522: DCHECK(CalledOnValidThread()); On 2014/09/26 05:20:30, Pawel Osciak wrote: > On 2014/09/26 04:56:41, sandersd wrote: > > On 2014/09/26 04:25:07, Pawel Osciak wrote: > > > On 2014/09/25 19:18:52, sandersd wrote: > > > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > > > If you stopped the decoder thread here, you wouldn't have to ensure it > was > > > > > defined last in the class. > > > > > > > > This is indeed possible, as long as the FlushTask is queued first, but it > > > > duplicates the implementation of QueueAction(). I'd like to think about it > > for > > > a > > > > while as a possible future refactoring. > > > > > > I don't understand the part about FlushTask... Queued first before what? > > > > Queued to the decoder thread message loop, before shutting down the decoder > > thread (so that the flush method gets called on VT, which is only done from > the > > decoder thread). > > If you decoder_thread_.Stop() from Destroy(), it will wait until all tasks are > done on decoder_thread_. If you require a FlushTask to always be done on > Destroy, just queue it explicitly here and then Stop()? This makes sense, but I'd still rather hold off. In particular, it can't go after QueueAction(), because QueueAction() may delete |this|, and I'd rather not just duplicate the contents of QueueAction() here. The good news is that I found a bug while analyzing this, QueueAction() could attempt to check for pending actions after ProcessDecodedFrames() deletes |this|. So that's fixed at least (at the cost of removing a minor optimization). https://codereview.chromium.org/491163002/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:531: QueueAction(ACTION_DESTROY); On 2014/09/26 05:20:30, Pawel Osciak wrote: > On 2014/09/26 04:56:41, sandersd wrote: > > On 2014/09/26 04:25:07, Pawel Osciak wrote: > > > On 2014/09/25 19:18:52, sandersd wrote: > > > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > > > Do we actually need this? You dropped everything already above anyway. > > > > > > > > We tell the client we are done with the buffers, but VT may still have to > > > flush > > > > (and while it's doing that, it will be calling back with a reference to > > > |this|). > > > > It may be possible to destroy the VT session and avoid that, but I'll need > > to > > > > ask the Apple engineers if that is guaranteed to work (fits with the > > stopping > > > > the decoder thread refactor from above). > > > > > > How do you guarantee that during that client_ will not be called anymore? > > > > During destroy, ProcessDecodedFrames() specifically does not call > SendPictures() > > or NotifyEndOfBitstreamBuffer(), but instead drops all the frames without any > > bookkeeping. > > Could you tell me how this is achieved please? Because there is a queued ACTION_DESTROY, it always goes through the drop frames path until all frames are decoded.
Thanks. ProcessDecodedFrames is much more readable now. lgtm % nits, give that you'd need another state member, keeping ACTION_DESTROY as is is an acceptable option. https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:522: DCHECK(CalledOnValidThread()); On 2014/09/26 06:56:02, sandersd wrote: > On 2014/09/26 05:20:30, Pawel Osciak wrote: > > On 2014/09/26 04:56:41, sandersd wrote: > > > On 2014/09/26 04:25:07, Pawel Osciak wrote: > > > > On 2014/09/25 19:18:52, sandersd wrote: > > > > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > > > > If you stopped the decoder thread here, you wouldn't have to ensure it > > was > > > > > > defined last in the class. > > > > > > > > > > This is indeed possible, as long as the FlushTask is queued first, but > it > > > > > duplicates the implementation of QueueAction(). I'd like to think about > it > > > for > > > > a > > > > > while as a possible future refactoring. > > > > > > > > I don't understand the part about FlushTask... Queued first before what? > > > > > > Queued to the decoder thread message loop, before shutting down the decoder > > > thread (so that the flush method gets called on VT, which is only done from > > the > > > decoder thread). > > > > If you decoder_thread_.Stop() from Destroy(), it will wait until all tasks are > > done on decoder_thread_. If you require a FlushTask to always be done on > > Destroy, just queue it explicitly here and then Stop()? > > This makes sense, but I'd still rather hold off. In particular, it can't go > after QueueAction(), because QueueAction() may delete |this|, and I'd rather not > just duplicate the contents of QueueAction() here. > The idea was to do away with ACTION_DESTROY. It wouldn't have been a problem then, although I guess then you'd need something else like a bool "destroying_" or something... https://chromiumcodereview.appspot.com/491163002/diff/140001/content/common/g... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/491163002/diff/140001/content/common/g... content/common/gpu/media/vt_video_decode_accelerator.cc:440: // Handle ACTION_DESTORY specially; it is an error to access any members s/DESTORY/DESTROY/ https://chromiumcodereview.appspot.com/491163002/diff/140001/content/common/g... content/common/gpu/media/vt_video_decode_accelerator.cc:443: CompleteAction(ACTION_DESTROY); This can't go under the switch?
https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/491163002/diff/60001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:522: DCHECK(CalledOnValidThread()); On 2014/09/26 15:30:49, Pawel Osciak wrote: > On 2014/09/26 06:56:02, sandersd wrote: > > On 2014/09/26 05:20:30, Pawel Osciak wrote: > > > On 2014/09/26 04:56:41, sandersd wrote: > > > > On 2014/09/26 04:25:07, Pawel Osciak wrote: > > > > > On 2014/09/25 19:18:52, sandersd wrote: > > > > > > On 2014/09/25 01:27:29, Pawel Osciak wrote: > > > > > > > If you stopped the decoder thread here, you wouldn't have to ensure > it > > > was > > > > > > > defined last in the class. > > > > > > > > > > > > This is indeed possible, as long as the FlushTask is queued first, but > > it > > > > > > duplicates the implementation of QueueAction(). I'd like to think > about > > it > > > > for > > > > > a > > > > > > while as a possible future refactoring. > > > > > > > > > > I don't understand the part about FlushTask... Queued first before what? > > > > > > > > Queued to the decoder thread message loop, before shutting down the > decoder > > > > thread (so that the flush method gets called on VT, which is only done > from > > > the > > > > decoder thread). > > > > > > If you decoder_thread_.Stop() from Destroy(), it will wait until all tasks > are > > > done on decoder_thread_. If you require a FlushTask to always be done on > > > Destroy, just queue it explicitly here and then Stop()? > > > > This makes sense, but I'd still rather hold off. In particular, it can't go > > after QueueAction(), because QueueAction() may delete |this|, and I'd rather > not > > just duplicate the contents of QueueAction() here. > > > > The idea was to do away with ACTION_DESTROY. It wouldn't have been a problem > then, although I guess then you'd need something else like a bool "destroying_" > or something... Acknowledged. https://chromiumcodereview.appspot.com/491163002/diff/140001/content/common/g... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/491163002/diff/140001/content/common/g... content/common/gpu/media/vt_video_decode_accelerator.cc:440: // Handle ACTION_DESTORY specially; it is an error to access any members On 2014/09/26 15:30:49, Pawel Osciak wrote: > s/DESTORY/DESTROY/ Done. https://chromiumcodereview.appspot.com/491163002/diff/140001/content/common/g... content/common/gpu/media/vt_video_decode_accelerator.cc:443: CompleteAction(ACTION_DESTROY); On 2014/09/26 15:30:49, Pawel Osciak wrote: > This can't go under the switch? Done.
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/491163002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as e0177ce8bec5f56627805fe3a0e2014dfc7e0a31
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/063dbc21c6cef08fcfec2bce1290d76358a323ba Cr-Commit-Position: refs/heads/master@{#296980} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
