[Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification

[Tomasz Figa]

On Wed, Dec 18, 2019 at 10:40 PM Gerd Hoffmann <kraxel at redhat.com> 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. Moreover, with certain features of certain codecs (e.g.
framebuffer reordering in H.264), there could be decoded frames that
need to be held by decoder, because later bitstream may contain
earlier frames.

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. From our experience
that was the easiest thing to deal with from both the driver and the
application, so I believe the same should apply to the host device
implementation and the guest driver.

>
> That is the only issue I've spotted in the protocol on a first quick
> look.  There are a few places where the spec text could be improved.
> I'll try to set aside some time to go over this in detail, but I can't
> promise I'll find the time to do that before xmas and new year.

Thanks a lot for review.

Best regards,
Tomasz