Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2408703002:
V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. (Closed)
DescriptionV4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD.
The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD
with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output
buffer with V4L2_BUF_FLAG_LAST.
BUG=558206
TEST=Run VDA unittest and CTS on elm.
Run VDA unittest on peach pi and nyan big.
Committed: https://crrev.com/37eabb62565c942a94dcf27e9d35867d34eec4d5
Cr-Commit-Position: refs/heads/master@{#426488}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 6
Patch Set 3 : address kcwu's comments #
Total comments: 4
Patch Set 4 : address kcwu's comments. Remove V4L2_DEC_CMD_START #
Total comments: 1
Patch Set 5 : add V4L2_DEC_CMD_START back because it works now #
Total comments: 34
Patch Set 6 : address Kuang-che's comments #
Total comments: 14
Patch Set 7 : address Pawel and Kuang-che's comments #
Messages
Total messages: 32 (14 generated)
wuchengli@chromium.org changed reviewers: + kcwu@chromium.org, posciak@chromium.org
PTAL
https://codereview.chromium.org/2408703002/diff/20001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/20001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:1407: IOCTL_OR_ERROR_RETURN(VIDIOC_DECODER_CMD, &cmd); IOCTL_OR_ERROR_RETURN_FALSE. wondering why compiler cannot catch this :( https://codereview.chromium.org/2408703002/diff/20001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:1642: bool V4L2VideoDecodeAccelerator::DecoderCmdSupported() { How about named it as IsDecoderCmdSupported() ? https://codereview.chromium.org/2408703002/diff/20001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:1780: flush_waiting_last_output_buffer_ = false; I'm wondering, if it is valid to call Reset() when Flush() is invoked but before FlushDone(), should flush_waiting_last_output_buffer_ reset to false in ResetTask as well?
https://codereview.chromium.org/2408703002/diff/20001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/20001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:1407: IOCTL_OR_ERROR_RETURN(VIDIOC_DECODER_CMD, &cmd); On 2016/10/11 08:46:05, kcwu wrote: > IOCTL_OR_ERROR_RETURN_FALSE. > wondering why compiler cannot catch this :( Fixed. https://codereview.chromium.org/2408703002/diff/20001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:1642: bool V4L2VideoDecodeAccelerator::DecoderCmdSupported() { On 2016/10/11 08:46:05, kcwu wrote: > How about named it as IsDecoderCmdSupported() ? Done. https://codereview.chromium.org/2408703002/diff/20001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:1780: flush_waiting_last_output_buffer_ = false; On 2016/10/11 08:46:05, kcwu wrote: > I'm wondering, if it is valid to call Reset() when Flush() is invoked but before > FlushDone(), > should flush_waiting_last_output_buffer_ reset > to false in ResetTask as well? I cleared flush_waiting_last_output_buffer_ in StopOutputStream(). I think that's the right place. ResetTask() calls StopOutputStream(). When the output queue is stopped, there's no need to wait any output buffer.
https://codereview.chromium.org/2408703002/diff/40001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/40001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:1400: if ((dqbuf.flags & V4L2_BUF_FLAG_LAST)) { IIUC, according to https://linuxtv.org/downloads/v4l-dvb-apis/vidioc-decoder-cmd.html, |bytesused| may be zero (line 1374), then this line won't be reached. https://codereview.chromium.org/2408703002/diff/40001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:1845: flush_waiting_last_output_buffer_ = false; This line won't be executed if output_streamon_ is false. IIUC, if the client calls 1. Initalize() 2. Flush() -> flush_waiting_last_output_buffer_ = true 3. not enqueue anything 4. Reset() -> flush_waiting_last_output_buffer_ not change
https://codereview.chromium.org/2408703002/diff/40001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/40001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:1400: if ((dqbuf.flags & V4L2_BUF_FLAG_LAST)) { On 2016/10/12 02:32:39, kcwu wrote: > IIUC, according to > https://linuxtv.org/downloads/v4l-dvb-apis/vidioc-decoder-cmd.html, |bytesused| > may be zero (line 1374), then this line won't be reached. Will fix this. https://codereview.chromium.org/2408703002/diff/40001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:1845: flush_waiting_last_output_buffer_ = false; On 2016/10/12 02:32:39, kcwu wrote: > This line won't be executed if output_streamon_ is false. > > IIUC, if the client calls > 1. Initalize() > 2. Flush() -> flush_waiting_last_output_buffer_ = true > 3. not enqueue anything > 4. Reset() -> flush_waiting_last_output_buffer_ not change FlushTask won't set flush_waiting_last_output_buffer_ to true if the state is kInitialized. I need to think what if the state is kResetting, kChangingResolution, or kAwaitingPictureBuffers in FlushTask.
Description was changed from ========== V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output buffer with V4L2_BUF_FLAG_LAST. The userspace starts the decoder by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_START. BUG=558206 TEST=Run VDA unittest and CTS on elm and peach pi. ========== to ========== V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output buffer with V4L2_BUF_FLAG_LAST. BUG=558206 TEST=Run VDA unittest and CTS on elm and peach pi. ==========
PTAL https://codereview.chromium.org/2408703002/diff/60001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/60001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:1375: if (errno == EPIPE) { We need this because this is not an error. It means no more output buffers can be dequeued.
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1651: cmd.cmd = V4L2_DEC_CMD_STOP; Do you need to add CMD_START back?
https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1578: // Queue up an empty buffer -- this triggers the flush. I found we could not do CMD_STOP here because we need to queue the existing input buffers first. So I moved CMD_STOP to line 837. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1651: cmd.cmd = V4L2_DEC_CMD_STOP; On 2016/10/17 16:24:52, kcwu wrote: > Do you need to add CMD_START back? No. I found VIDIOC_TRY_DECODER_CMD didn't mean if a command is supported. It means whether the driver can run the command at that time. When the device is just initialized, stream is off. So running CMD_STOP does nothing. But running CMD_START shouldn't work because stream is not on. https://linuxtv.org/downloads/v4l-dvb-apis/vidioc-decoder-cmd.html "When the decoder is already stopped, this command does nothing."
https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:838: flush_waiting_last_output_buffer_ = true; Do you need to set decoder_partial_frame_pending_=false here or line 1414? https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1562: if (!output_streamon_) { This change sounds not good because you do something before checking decoder_state_. How about move this to line 1577? https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1651: cmd.cmd = V4L2_DEC_CMD_STOP; On 2016/10/18 02:04:51, wuchengli wrote: > On 2016/10/17 16:24:52, kcwu wrote: > > Do you need to add CMD_START back? > No. I found VIDIOC_TRY_DECODER_CMD didn't mean if a command is supported. It > means whether the driver can run the command at that time. When the device is > just initialized, stream is off. So running CMD_STOP does nothing. But running > CMD_START shouldn't work because stream is not on. > > https://linuxtv.org/downloads/v4l-dvb-apis/vidioc-decoder-cmd.html > "When the decoder is already stopped, this command does nothing." I don't fully understand. Do you mean we can try CMD_STOP anytime but cannot try CMD_START certain time? If CMD_STOP has also timing constraint, please add DCHECK and add comment about that.
https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:831: if (decoder_cmd_supported_) { Pawel. It looks like I need to check schedule_task here. Right?
Description was changed from ========== V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output buffer with V4L2_BUF_FLAG_LAST. BUG=558206 TEST=Run VDA unittest and CTS on elm and peach pi. ========== to ========== V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output buffer with V4L2_BUF_FLAG_LAST. BUG=558206 TEST=Run VDA unittest and CTS on elm and nyan. ==========
Description was changed from ========== V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output buffer with V4L2_BUF_FLAG_LAST. BUG=558206 TEST=Run VDA unittest and CTS on elm and nyan. ========== to ========== V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output buffer with V4L2_BUF_FLAG_LAST. BUG=558206 TEST=Run VDA unittest and CTS on elm. Run VDA unittest on peach pi and nyan big. ==========
Description was changed from ========== V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output buffer with V4L2_BUF_FLAG_LAST. BUG=558206 TEST=Run VDA unittest and CTS on elm. Run VDA unittest on peach pi and nyan big. ========== to ========== V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output buffer with V4L2_BUF_FLAG_LAST. BUG=558206 TEST=Run VDA unittest and CTS on elm. Run VDA unittest on peach pi and nyan big. ==========
Kuang-che. PTAL. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:838: flush_waiting_last_output_buffer_ = true; On 2016/10/18 05:07:13, kcwu wrote: > Do you need to set decoder_partial_frame_pending_=false here or line 1414? We should set decoder_partial_frame_pending_=false here. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1562: if (!output_streamon_) { On 2016/10/18 05:07:13, kcwu wrote: > This change sounds not good because you do something before checking > decoder_state_. How about move this to line 1577? Done. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1651: cmd.cmd = V4L2_DEC_CMD_STOP; On 2016/10/18 05:07:13, kcwu wrote: > On 2016/10/18 02:04:51, wuchengli wrote: > > On 2016/10/17 16:24:52, kcwu wrote: > > > Do you need to add CMD_START back? > > No. I found VIDIOC_TRY_DECODER_CMD didn't mean if a command is supported. It > > means whether the driver can run the command at that time. When the device is > > just initialized, stream is off. So running CMD_STOP does nothing. But running > > CMD_START shouldn't work because stream is not on. > > > > https://linuxtv.org/downloads/v4l-dvb-apis/vidioc-decoder-cmd.html > > "When the decoder is already stopped, this command does nothing." > > I don't fully understand. Do you mean we can try CMD_STOP anytime but cannot try > CMD_START certain time? That's right. > > If CMD_STOP has also timing constraint, please add DCHECK and add comment about > that. I added some comments.
https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:840: schedule_task = true; this line is redundant if you already check it at line 831. https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:845: schedule_task = true; ah, this line too https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1420: struct v4l2_decoder_cmd cmd; DCHECK(decoder_cmd_supported_); https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1567: } add one blank line https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1568: if (!output_streamon_) { I guess I found a tricky case. Correct me if I'm wrong. When resolution change, we will streamoff & streamon temporaily. If we are in IMPORT mode, streamon and streamoff are not atomic. So, considering this case: 1. Enqueue some input 2. Resolution change -> streamoff 3. inside FinishResolutionChange, we will call Client::ProvidePictureBuffers 4. client call Flush before AssignPictureBuffers. we will notify flush done immediately. https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:293: // Returns true if V4L2_DEC_CMD_STOP is supported. s/V4L2_DEC_CMD_STOP/V4L2_DEC_CMD_*/. Since this function's intention is to check supportness in general. https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:457: // True if V4L2_DEC_CMD_STOP is supported. s/V4L2_DEC_CMD_STOP/V4L2_DEC_CMD_*/
https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_device.h File media/gpu/v4l2_device.h (left): https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_device.... media/gpu/v4l2_device.h:29: #define V4L2_PIX_FMT_VP8_FRAME v4l2_fourcc('V', 'P', '8', 'F') Why remove VP8F, but not S264? I think they should've both gone in at similar time? https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:292: decoder_cmd_supported_ = IsDecoderCmdSupported(); Ideally, we should try to run this on decoder_thread, especially since it's already running. For correctness and to offload gpu main. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:831: if (decoder_cmd_supported_) { On 2016/10/19 08:02:08, wuchengli wrote: > Pawel. It looks like I need to check schedule_task here. Right? Yes, but probably best to separate that logic in a separate if (i.e. the backpressure case). https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1375: if (errno == EPIPE) { else if? Or even if (errno == EAGAIN || errno == EPIPE) with an appropriate comment? https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1414: if ((dqbuf.flags & V4L2_BUF_FLAG_LAST)) { s/((/(/ s/))/)/ https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1417: if (flush_waiting_last_output_buffer_) { Would we perhaps always want to restart, even after EOS in the stream, in case the VDA client schedules more decodes after this? https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1422: flush_waiting_last_output_buffer_ = false; We could possibly set this at the beginning, since regardless of whether we return in 1421, we probably want to clear the flag anyway? https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1578: // Queue up an empty buffer -- this triggers the flush. On 2016/10/18 02:04:51, wuchengli wrote: > I found we could not do CMD_STOP here because we need to queue the existing > input buffers first. So I moved CMD_STOP to line 837. Perhaps it would be useful to add a comment about this somewhere in the code as well? https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1648: bool V4L2VideoDecodeAccelerator::IsDecoderCmdSupported() { Could we DCHECK we are on decoder thread here? https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:457: // True if V4L2_DEC_CMD_STOP are supported. s/V4L2_DEC_CMD_STOP/VIDIOC_DECODER_CMD/ ? s/are/is/ https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:459: // True if flushing is waiting for last output buffer. It would be great to add a bit more documentation how this flag is used please. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:460: bool flush_waiting_last_output_buffer_; Nit: s/waiting/awaiting/
Patchset #7 (id:140001) has been deleted
All done. PTAL. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_device.h File media/gpu/v4l2_device.h (left): https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_device.... media/gpu/v4l2_device.h:29: #define V4L2_PIX_FMT_VP8_FRAME v4l2_fourcc('V', 'P', '8', 'F') On 2016/10/19 09:23:04, Pawel Osciak wrote: > Why remove VP8F, but not S264? I think they should've both gone in at similar > time? You are right. I checked again and S264 is in third_party/chromiumos-overlay/sys-kernel/linux-headers/files/0001-CHROMIUM-media-headers-Import-V4L2-headers-from-Chro.patch Third party code! Follow proper procedures. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:292: decoder_cmd_supported_ = IsDecoderCmdSupported(); On 2016/10/19 09:23:04, Pawel Osciak wrote: > Ideally, we should try to run this on decoder_thread, especially since it's > already running. For correctness and to offload gpu main. I've moved it before decoder_thread start. Many other ioctls are run in this function in child thread. If we want to run this on decoder_thread, all the ioctls in this function should be moved as well. That should be done in another CL. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:831: if (decoder_cmd_supported_) { On 2016/10/19 09:23:04, Pawel Osciak wrote: > On 2016/10/19 08:02:08, wuchengli wrote: > > Pawel. It looks like I need to check schedule_task here. Right? > > Yes, but probably best to separate that logic in a separate if (i.e. the > backpressure case). Done. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1375: if (errno == EPIPE) { On 2016/10/19 09:23:04, Pawel Osciak wrote: > else if? > > Or even if (errno == EAGAIN || errno == EPIPE) with an appropriate comment? Changed to else if https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1414: if ((dqbuf.flags & V4L2_BUF_FLAG_LAST)) { On 2016/10/19 09:23:04, Pawel Osciak wrote: > s/((/(/ > s/))/)/ Done. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1417: if (flush_waiting_last_output_buffer_) { On 2016/10/19 09:23:04, Pawel Osciak wrote: > Would we perhaps always want to restart, even after EOS in the stream, in case > the VDA client schedules more decodes after this? Your V4L2 spec doesn't way we need to do CMD_START during resolution change. During resolution change, STREAMOFF and STREAMON will be called. So implicitly STOP and START will be sent. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1422: flush_waiting_last_output_buffer_ = false; On 2016/10/19 09:23:04, Pawel Osciak wrote: > We could possibly set this at the beginning, since regardless of whether we > return in 1421, we probably want to clear the flag anyway? Done. It doesn't matter much because VDA will be destroyed soon after notifying an error. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1578: // Queue up an empty buffer -- this triggers the flush. On 2016/10/19 09:23:04, Pawel Osciak wrote: > On 2016/10/18 02:04:51, wuchengli wrote: > > I found we could not do CMD_STOP here because we need to queue the existing > > input buffers first. So I moved CMD_STOP to line 837. > > Perhaps it would be useful to add a comment about this somewhere in the code as > well? Explained in Enqueue. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1648: bool V4L2VideoDecodeAccelerator::IsDecoderCmdSupported() { On 2016/10/19 09:23:04, Pawel Osciak wrote: > Could we DCHECK we are on decoder thread here? Explained in the above comment. This still runs in the child thread. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:457: // True if V4L2_DEC_CMD_STOP are supported. On 2016/10/19 09:23:04, Pawel Osciak wrote: > s/V4L2_DEC_CMD_STOP/VIDIOC_DECODER_CMD/ ? > s/are/is/ Done. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:459: // True if flushing is waiting for last output buffer. On 2016/10/19 09:23:05, Pawel Osciak wrote: > It would be great to add a bit more documentation how this flag is used please. Done. https://codereview.chromium.org/2408703002/diff/100001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:460: bool flush_waiting_last_output_buffer_; On 2016/10/19 09:23:05, Pawel Osciak wrote: > Nit: s/waiting/awaiting/ Done. https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:840: schedule_task = true; On 2016/10/19 09:18:21, kcwu wrote: > this line is redundant if you already check it at line 831. I added this to be consistent with line 845. Removed. https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:845: schedule_task = true; On 2016/10/19 09:18:21, kcwu wrote: > ah, this line too Removed. https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1420: struct v4l2_decoder_cmd cmd; On 2016/10/19 09:18:21, kcwu wrote: > DCHECK(decoder_cmd_supported_); This is not needed. VIDIOC_DECODER_CMD will fail gracefully. We will see the log and fix the bug. https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1567: } On 2016/10/19 09:18:21, kcwu wrote: > add one blank line The code was moved. https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:1568: if (!output_streamon_) { On 2016/10/19 09:18:21, kcwu wrote: > I guess I found a tricky case. Correct me if I'm wrong. > > When resolution change, we will streamoff & streamon temporaily. If we are in > IMPORT mode, streamon and streamoff are not atomic. > > So, considering this case: > 1. Enqueue some input > 2. Resolution change -> streamoff > 3. inside FinishResolutionChange, we will call Client::ProvidePictureBuffers > 4. client call Flush before AssignPictureBuffers. > we will notify flush done immediately. As discussed offline, only send CMD_STOP when all input are dequeued. https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:293: // Returns true if V4L2_DEC_CMD_STOP is supported. On 2016/10/19 09:18:21, kcwu wrote: > s/V4L2_DEC_CMD_STOP/V4L2_DEC_CMD_*/. > Since this function's intention is to check supportness in general. Changed to VIDIOC_DECODER_CMD. https://codereview.chromium.org/2408703002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.h:457: // True if V4L2_DEC_CMD_STOP is supported. On 2016/10/19 09:18:21, kcwu wrote: > s/V4L2_DEC_CMD_STOP/V4L2_DEC_CMD_*/ Changed to VIDIOC_DECODER_CMD.
The CQ bit was checked by wuchengli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by wuchengli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output buffer with V4L2_BUF_FLAG_LAST. BUG=558206 TEST=Run VDA unittest and CTS on elm. Run VDA unittest on peach pi and nyan big. ========== to ========== V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output buffer with V4L2_BUF_FLAG_LAST. BUG=558206 TEST=Run VDA unittest and CTS on elm. Run VDA unittest on peach pi and nyan big. ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output buffer with V4L2_BUF_FLAG_LAST. BUG=558206 TEST=Run VDA unittest and CTS on elm. Run VDA unittest on peach pi and nyan big. ========== to ========== V4L2VideoDecodeAccelerator: implement flush by VIDIOC_DECODER_CMD. The official V4L2 flush begins by issuing VIDIOC_DECODER_CMD with cmd=V4L2_DEC_CMD_STOP. The driver will set the last output buffer with V4L2_BUF_FLAG_LAST. BUG=558206 TEST=Run VDA unittest and CTS on elm. Run VDA unittest on peach pi and nyan big. Committed: https://crrev.com/37eabb62565c942a94dcf27e9d35867d34eec4d5 Cr-Commit-Position: refs/heads/master@{#426488} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/37eabb62565c942a94dcf27e9d35867d34eec4d5 Cr-Commit-Position: refs/heads/master@{#426488} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
