V4L2 decoder failed in resolution change during decoding

[Nicolas Dufresne]

Hi Hsia-Jun,

Le jeudi 11 mai 2023 à 18:26 +0800, Hsia-Jun Li a écrit :
> Hello Nicolas
> 
> We meet a bug from  !4437, if the video have multiple sequences or we 
> could simple call it would lead to resolution change many times.
> 
> How this bug happens is below, I believe the driver follows 
> Memory-to-Memory Stateful Video Decoder Interface.
> 
> 1. seq 1 last frame N
> 
> and frame N is in its display order
> 
> 2. hardware begin to process next input buffer
> 
> 3. hardware found a new seq
> 
> 4. driver send out V4L2_EVENT_SRC_CH_RESOLUTION event
> 
> 5. driver dequeue a buffer which bytesued = 0 in all its planes and 
> V4L2_BUF_FLAG_LAST flag.

This specific condition is a bit unexpected. As a driver, it seems weird to wait
for a free capture buffer to signal that the last buffer has been handled in the
past. In fact, all the driver I'm working on will basically set the flag on the
real CAPTURE buffer if its not dequeud yet, otherwise they set 
q->last_buffer_dequeued otherwise.

That being said, I'm fine if we add support for this special case. Right now it
exits for legacy reasons (legacy drain flow). I think the way forward could be
to just ignore that buffer if we have both payload == 0 and V4L2_BUF_FLAG_LAST
(logic being that V4L2_BUF_FLAG_LAST did not exist in legacy drivers, hence
cannot be there). GStreamer will just poll again, and DQBUF -> EPIPE, getting
you back on track.

> 
> If gst_v4l2_video_dec_loop() dequeue the CAPTURE pool in both step 4 and 5.
> 
> It would pause the srcpad thread while the self->output_flow would be 
> GST_V4L2_FLOW_LAST_BUFFER (GST_FLOW_CUSTOM_SUCCESS). That would cause 
> the gst_v4l2_video_dec_handle_frame() all reminded buffer.
> 
> 
> In my  !3456, this won't  happen. If you could confirm this bug, I would 
> create an issue(or update #1149) in the gitlab.

Unless mistaken, its an easy patch, better just send over a fix.

> 
> Besides, there are several things I would like to improve v4l2dec, like 
> #2023, #2024 and !3537.

#2023 is a long standing known issue. The conclusion should be that enumerating
colorimetry in V4L2 should not be attempted. That imply two possible solutions.

1. Just drop entirely the colorimetry and make the best possible match, with a
simple bus warning if no match was possible.

2. Keep the current partially enumerated colorimetry using try_fmt, but loosen 
the final caps so anything will endup being accepted.

1. is obviously cleaner and easier, but completely breaks the use case for which
this was introduced in the first place. In fact it was introduced for some TI
capture pipeline, where the capture pipeline set of supported colorimetry was
larger the what the encoder could support. This enumeration guarantied that a
valid colorimetry setup was picked. With 1., we are back to a random pick. With
2. though, we keep a limited set of verified colorimetry values, which would
maintain the original features. In short, 2. is backward compatible, were 1. is
not.

#2024 V4L2 stateful decoder support is missing TIER support

Answer: someone should implement V4L2_CID_MPEG_VIDEO_HEVC_TIER support into
GStreamer. I suspect Philippe Normand might have done a blind implemention back
then, specially that browsers don't often support HEVC.

!3537 Variable framerate

It's submitter job to promote their patches. I didn't have this patch in my
todos, and there haven't been any activity in this MR in the last five months to
help reminding me. I'll make a code review now that I'm notified of it.

> 
> All those related to the v4l2 caps probing, if that is ok I would begin 
> my work on writing this.
> 
> Also I think the way we handle the buffer from MMAP allocator is not 
> good, it would lead to memory copy as always. Some downstream could 
> accept DMA buffer even they don't support the DMA buffer allocator, in 
> performance evaluation, we would use a component like fakesink, even the 
> buffer can't be read through memory mapping is not a matter, fakesink 
> would not read it at all.

Consider testing with fakevideosink which implement GstVideoMeta to prevent a
copy. The rational for the copy is that the strides needs to be standardise if
downstream does not support GstVideoMeta. So if needed, a copy will be made
before fakesink or filesink. Note that there is a  bug were it fails to detect
padding, and may have the buffer size wrong. Hopefully someone will find the
time to fix it this year, its a rare use case though.

> 
> Could I change this buffer mechanism? If it is possible, I would like to 
> do that after my proposal for v4l2 pix and buffer ext APIs.

You haven't demonstrated the need for it yet.

regards,
Nicolas