Re: [PATCH v5 1/5] [media] DocBook media: document mem2mem draining flow

[Philipp Zabel <p.zabel@pengutronix.de>]

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@pengutronix.de>
> > ---
> > 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>

regards
Philipp