On 05/04/2015 12:21 PM, Philipp Zabel wrote: > Hi Hans, Kamil, > > thank you for your comments. > > Am Montag, den 27.04.2015, 15:23 +0200 schrieb Hans Verkuil: >> Hi Philipp, >> >> I finally got around to reviewing this patch series. Sorry for the delay, but >> here are my comments: >> >> On 04/20/2015 10:28 AM, Philipp Zabel wrote: >>> Document the interaction between VIDIOC_DECODER_CMD V4L2_DEC_CMD_STOP and >>> VIDIOC_ENCODER_CMD V4L2_ENC_CMD_STOP to start the draining, the V4L2_EVENT_EOS >>> event signalling all capture buffers are finished and ready to be dequeud, >>> the new V4L2_BUF_FLAG_LAST buffer flag indicating the last buffer being dequeued >>> from the capture queue, and the poll and VIDIOC_DQBUF ioctl return values once >>> the queue is drained. >>> >>> Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> >>> --- >>> Changes since v4: >>> - Split out documentation changes into a separate patch >>> - Changed wording following Pawel's suggestions. >>> --- >>> Documentation/DocBook/media/v4l/io.xml | 10 ++++++++++ >>> Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml | 9 ++++++++- >>> Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml | 8 +++++++- >>> Documentation/DocBook/media/v4l/vidioc-qbuf.xml | 8 ++++++++ >>> 4 files changed, 33 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml >>> index 1c17f80..f561037 100644 >>> --- a/Documentation/DocBook/media/v4l/io.xml >>> +++ b/Documentation/DocBook/media/v4l/io.xml >>> @@ -1129,6 +1129,16 @@ in this buffer has not been created by the CPU but by some DMA-capable unit, >>> in which case caches have not been used.</entry> >>> </row> >>> <row> >>> + <entry><constant>V4L2_BUF_FLAG_LAST</constant></entry> >>> + <entry>0x00100000</entry> >>> + <entry>Last buffer produced by the hardware. mem2mem codec drivers >>> +set this flag on the capture queue for the last buffer when the >>> +<link linkend="vidioc-querybuf">VIDIOC_QUERYBUF</link> or >>> +<link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl is called. Any subsequent >>> +call to the <link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl will not block >>> +anymore, but return an &EPIPE;.</entry> >> >> As Kamil mentioned in his review, we should allow for bytesused == 0 here. > > How about: > > @@ -1134,9 +1134,11 @@ in which case caches have not been used.</entry> > <entry>Last buffer produced by the hardware. mem2mem codec drivers > set this flag on the capture queue for the last buffer when the > <link linkend="vidioc-querybuf">VIDIOC_QUERYBUF</link> or > -<link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl is called. Any subsequent > -call to the <link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl will not block > -anymore, but return an &EPIPE;.</entry> > +<link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl is called. Due to hardware > +limitations, the last buffer may be empty. In this case the driver will set the > +<structfield>bytesused</structfield> field to 0, regardless of the format. Any > +Any subsequent call to the <link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl > +will not block anymore, but return an &EPIPE;.</entry> > </row> > <row> > <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant></entry> > >>> + </row> >>> + <row> >>> <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant></entry> >>> <entry>0x0000e000</entry> >>> <entry>Mask for timestamp types below. To test the >>> diff --git a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml >>> index 9215627..6502d82 100644 >>> --- a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml >>> +++ b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml >>> @@ -197,7 +197,14 @@ be muted when playing back at a non-standard speed. >>> this command does nothing. This command has two flags: >>> if <constant>V4L2_DEC_CMD_STOP_TO_BLACK</constant> is set, then the decoder will >>> set the picture to black after it stopped decoding. Otherwise the last image will >>> -repeat. If <constant>V4L2_DEC_CMD_STOP_IMMEDIATELY</constant> is set, then the decoder >>> +repeat. mem2mem decoders will stop producing new frames altogether. They will send >>> +a <constant>V4L2_EVENT_EOS</constant> event when the last frame has been decoded >>> +and all frames are ready to be dequeued and will set the >>> +<constant>V4L2_BUF_FLAG_LAST</constant> buffer flag on the last buffer of the >> >> Make a note here as well that the last buffer might be an empty buffer. > > @@ -201,9 +201,12 @@ repeat. mem2mem decoders will stop producing new frames altogether. They will se > a <constant>V4L2_EVENT_EOS</constant> event when the last frame has been decoded > and all frames are ready to be dequeued and will set the > <constant>V4L2_BUF_FLAG_LAST</constant> buffer flag on the last buffer of the > -capture queue to indicate there will be no new buffers produced to dequeue. Once > -this flag was set, the <link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl > -will not block anymore, but return an &EPIPE;. > +capture queue to indicate there will be no new buffers produced to dequeue. This > +buffer may be empty, indicated by the driver setting the > +<structfield>bytesused</structfield> field to 0. Once the > +<constant>V4L2_BUF_FLAG_LAST</constant> flag was set, the > +<link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl will not block anymore, > +but return an &EPIPE;. > If <constant>V4L2_DEC_CMD_STOP_IMMEDIATELY</constant> is set, then the decoder > stops immediately (ignoring the <structfield>pts</structfield> value), otherwise it > will keep decoding until timestamp >= pts or until the last of the pending data from > >>> +capture queue to indicate there will be no new buffers produced to dequeue. Once >>> +this flag was set, the <link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl >>> +will not block anymore, but return an &EPIPE;. >>> +If <constant>V4L2_DEC_CMD_STOP_IMMEDIATELY</constant> is set, then the decoder >>> stops immediately (ignoring the <structfield>pts</structfield> value), otherwise it >>> will keep decoding until timestamp >= pts or until the last of the pending data from >>> its internal buffers was decoded. >>> diff --git a/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml b/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml >>> index 0619ca5..3cdb841 100644 >>> --- a/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml >>> +++ b/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml >>> @@ -129,7 +129,13 @@ this command.</entry> >>> encoding will continue until the end of the current <wordasword>Group >>> Of Pictures</wordasword>, otherwise encoding will stop immediately. >>> When the encoder is already stopped, this command does >>> -nothing.</entry> >>> +nothing. mem2mem encoders will send a <constant>V4L2_EVENT_EOS</constant> event >>> +when the last frame has been decoded and all frames are ready to be dequeued and >>> +will set the <constant>V4L2_BUF_FLAG_LAST</constant> buffer flag on the last >>> +buffer of the capture queue to indicate there will be no new buffers produced to >> >> Ditto. > > @@ -133,7 +133,9 @@ nothing. mem2mem encoders will send a <constant>V4L2_EVENT_EOS</constant> event > when the last frame has been decoded and all frames are ready to be dequeued and > will set the <constant>V4L2_BUF_FLAG_LAST</constant> buffer flag on the last > buffer of the capture queue to indicate there will be no new buffers produced to > -dequeue. Once this flag was set, the > +dequeue. This buffer may be empty, indicated by the driver setting the > +<structfield>bytesused</structfield> field to 0. Once the > +<constant>V4L2_BUF_FLAG_LAST</constant> flag was set, the > <link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl will not block anymore, > but return an &EPIPE;.</entry> > </row> Looks good to me. With this change: Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> Regards, Hans > > regards > Philipp > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
