Hi Tomasz, On Donnerstag, 19. Dezember 2019 10:59:02 CET Tomasz Figa wrote: > On Thu, Dec 19, 2019 at 6:48 PM Dmitry Sepp <dmitry.sepp@xxxxxxxxxxxxxxx> wrote: > > Hi, > > > > On Donnerstag, 19. Dezember 2019 08:46:39 CET Gerd Hoffmann wrote: > > > On Wed, Dec 18, 2019 at 11:08:37PM +0900, Tomasz Figa wrote: > > > > On Wed, Dec 18, 2019 at 10:40 PM Gerd Hoffmann <kraxel@xxxxxxxxxx> wrote: > > > > > Hi, > > > > > > > > > > > +The device MUST mark the last buffer with the > > > > > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > > > > > +sequence. > > > > > > > > > > No, that would build a race condition into the protocol. The device > > > > > could complete the last buffer after the driver has sent the drain > > > > > command but before the device saw it. So the flag would not be > > > > > reliable. > > > > > > > > > > I also can't see why the flag is needed in the first place. The > > > > > driver > > > > > should know which buffers are queued still and be able to figure > > > > > whenever the drain is complete or not without depending on that > > > > > flag. > > > > > So I'd suggest to simply drop it. > > > > > > > > Unfortunately video decoders are not that simple. There are always > > > > going to be some buffers on the decoder side used as reference frames. > > > > Only the decoder knows when to release them, as it continues decoding > > > > the stream. > > > > > > Not clearly defined in the spec: When is the decoder supposed to send > > > the response for a queue request? When it finished decoding (i.e. frame > > > is ready for playback), or when it doesn't need the buffer any more for > > > decoding (i.e. buffer can be re-queued or pages can be released)? > > > > In my eyes the both statements mean almost the same and both are valid. I > > think whatever underlying libraries are used for decoding on the device > > side, they simply won't return us the buffer as long as the HW device > > needs them to continue its normal operation. So your first sentence > > applies to output buffers, the second - to input buffers. > > > > My understanding is as follows: we send the response for a queue request > > as > > soon as the HW device on the host side passes the buffer ownership back to > > the client (like when VIDIOC_DQBUF has returned a buffer). > > That's how it's defined in V4L2 and what makes the most sense from the > video decoding point of view, as one wants to display frames as soon > as they are available. > > However that still doesn't let the driver know which buffers will be > dequeued when. A simple example of this scenario is when the guest is > done displaying a frame and requeues the buffer back to the decoder. > Then the decoder will not choose it for decoding next frames into as > long as the frame in that buffer is still used as a reference frame, > even if one sends the drain request. It might be that I'm getting your point wrong, but do you mean some hardware can mark a buffer as ready to be displayed yet still using the underlying memory to decode other frames? This means, if you occasionally/intentionally write to the buffer you mess up the whole decoding pipeline. That would be strange at least... Regds, Dmitry. > > > Thanks for reviewing! > > > > Regards, > > Dmitry. > > > > > > How we defined this in the V4L2 stateful decoder interface is that if > > > > the decoder happened to return the last framebuffer before the drain > > > > request arrived, it would return one more, empty. > > > > > > Ok. That is not clear from the spec. I've read the drain request as as > > > "please stop decoding and complete all queue requests which are in the > > > virtqueue, even when you didn't process them yet". In which case > > > completing the last pending queue request would clearly indicate the > > > draining request is complete. Also completing the drain request should > > > only happen when the operation is finished. > > > > > > I think the various states a buffer can have and how queuing & deciding > > > & draining changes these states should be clarified in the > > > specification. > > > > > > cheers, > > > > > > Gerd
