On 12/19/2018 08:16 AM, Tomasz Figa wrote: > On Wed, Dec 19, 2018 at 4:04 PM Jonas Karlman <jonas@xxxxxxxxx> wrote: >> >> On 2018-12-19 06:10, Tomasz Figa wrote: >>> On Thu, Dec 13, 2018 at 9:28 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: >>>> On 12/12/18 7:28 PM, Jonas Karlman wrote: >>>>> Hi Hans, >>>>> >>>>> Since this function only return DEQUEUED and DONE buffers, >>>>> it cannot be used to find a capture buffer that is both used for >>>>> frame output and is part of the frame reference list. >>>>> E.g. a bottom field referencing a top field that is already >>>>> part of the capture buffer being used for frame output. >>>>> (top and bottom field is output in same buffer) >>>>> >>>>> Jernej Škrabec and me have worked around this issue in cedrus driver by >>>>> first checking >>>>> the tag/timestamp of the current buffer being used for output frame. >>>>> >>>>> >>>>> // field pictures may reference current capture buffer and is not >>>>> returned by vb2_find_tag >>>>> if (v4l2_buf->tag == dpb->tag) >>>>> buf_idx = v4l2_buf->vb2_buf.index; >>>>> else >>>>> buf_idx = vb2_find_tag(cap_q, dpb->tag, 0); >>>>> >>>>> >>>>> What is the recommended way to handle such case? >>>> That is the right approach for this. Interesting corner case, I hadn't >>>> considered that. >>>> >>>>> Could vb2_find_timestamp be extended to allow QUEUED buffers to be returned? >>>> No, because only the driver knows what the current buffer is. >>>> >>>> Buffers that are queued to the driver are in state ACTIVE. But there may be >>>> multiple ACTIVE buffers and vb2 doesn't know which buffer is currently >>>> being processed by the driver. >>>> >>>> So this will have to be checked by the driver itself. >>> Hold on, it's a perfectly valid use case to have the buffer queued but >>> still used as a reference for previously queued buffers, e.g. >>> >>> QBUF(O, 0) >>> QBUF(C, 0) >>> REF(ref0, out_timestamp(0)) >>> QBUF(O, 1) >>> QBUF(C, 1) >>> REF(ref0, out_timestamp(0)) >>> QBUF(O, 2) >>> QBUF(C, 2) >>> <- driver returns O(0) and C(0) here >>> <- userspace also knows that any next frame will not reference C(0) anymore >>> REF(ref0, out_timestamp(2)) >>> QBUF(O, 0) >>> QBUF(C, 0) >>> <- driver may pick O(1)+C(1) or O(2)+C(2) to decode here, but C(0) >>> which is the reference for it is already QUEUED. >>> >>> It's a perfectly fine scenario and optimal from pipelining point of >>> view, but if I'm not missing something, the current patch wouldn't >>> allow it. >> >> This scenario should never happen with FFmpeg + v4l2request hwaccel + >> Kodi userspace. >> FFmpeg would only QBUF O(0)+C(0) again after it has been presented on >> screen and Kodi have released the last reference to the AVFrame. >> > > I skipped the display in the example indeed, but we can easily add it: > > QBUF(O, 0) > QBUF(C, 0) > REF(ref0, out_timestamp(0)) > QBUF(O, 1) > QBUF(C, 1) > <- driver returns O(0) and C(0) here > <- userspace displays C(0) > REF(ref0, out_timestamp(0)) > QBUF(O, 2) > QBUF(C, 2) > REF(ref0, out_timestamp(0)) > QBUF(O, 3) > QBUF(C, 3) > <- driver returns O(1) and C(1) here > <- userspace displays C(1) and reclaims C(0) > <- userspace also knows that any next frame will not reference C(0) anymore > REF(ref0, out_timestamp(3)) > QBUF(O, 0) > QBUF(C, 0) When C(0) is queued its timestamp field is zeroed by vb2. So it can no longer be used as a reference. For now I want to keep the behavior like that (i.e. once you requeue a capture buffer it can no longer be used as a reference frame for the decoder). This limitation is something we might want to lift in the future, but this would require more work internally. Regards, Hans > <- driver may pick O(3)+C(3) to decode here, but C(0) > which is the reference for it is already QUEUED. > > Also the fact that FFmpeg wouldn't trigger such case doesn't mean that > it's an invalid one. If I remember correctly, Chromium would actually > trigger such, since we attempt to pipeline things as much as possible. > >> The v4l2request hwaccel will keep a AVFrame pool with preallocated >> frames, AVFrame(x) is keeping userspace ref to O(x)+C(x). >> An AVFrame will not be released back to the pool until FFmpeg have >> removed it from DPB and Kodi have released it after it no longer is >> being presented on screen. >> >> E.g. an IPBIPB sequense with display order 0 2 1 3 5 4 >> >> FFmpeg: AVFrame(0) >> QBUF: O(0)+C(0) >> DQBUF: O(0)+C(0) >> Kodi: AVFrame(0) returned from FFmpeg and presented on screen >> FFmpeg: AVFrame(1) with ref to AVFrame(0) >> QBUF: O(1)+C(1) with ref to timestamp(0) >> DQBUF: O(1)+C(1) >> FFmpeg: AVFrame(2) with ref to AVFrame(0)+AVFrame(1) >> QBUF: O(2)+C(2) with ref to timestamp(0)+timestamp(1) >> DQBUF: O(2)+C(2) >> Kodi: AVFrame(2) returned from FFmpeg and presented on screen >> Kodi: AVFrame(0) released (no longer presented) >> FFmpeg: AVFrame(3) >> QBUF: O(3)+C(3) >> DQBUF: O(3)+C(3) >> Kodi: AVFrame(1) returned from FFmpeg and presented on screen >> Kodi: AVFrame(2) released (no longer presented) >> FFmpeg: AVFrame(2) returned to pool >> FFmpeg: AVFrame(2) with ref to AVFrame(3) >> QBUF: O(2)+C(2) with ref to timestamp(3) >> DQBUF: O(2)+C(2) >> Kodi: AVFrame(3) returned from FFmpeg and presented on screen >> Kodi: AVFrame(1) released (no longer presented) >> FFmpeg: AVFrame(0)+AVFrame(1) returned to pool (no longer referenced) >> FFmpeg: AVFrame(0) with ref to AVFrame(3)+AVFrame(2) >> QBUF: O(0)+C(0) with ref to timestamp(3)+timestamp(2) >> DQBUF: O(0)+C(0) >> Kodi: AVFrame(0) returned from FFmpeg and presented on screen >> Kodi: AVFrame(3) released (no longer presented) >> and so on >> >> Here we can see that O(0)+C(0) will not be QBUF until after FFmpeg + >> Kodi have released all userspace refs to AVFrame(0). >> Above example was simplified, Kodi will normally keep a few decoded >> frames in buffer before being presented and FFmpeg will CREATE_BUF >> anytime the pool is empty and new O/C buffers is needed. >> >> Regards, >> Jonas >> >>> Best regards, >>> Tomasz
