Re: [PATCH] media: coda: fix comparision of decoded frames' indexes

[Martin Kepplinger]

Am 22.11.2017 14:43 schrieb Philipp Zabel:
> Hi Martin,
>
> On Fri, 2017-11-17 at 15:30 +0100, Martin Kepplinger wrote:
> > At this point the driver looks the currently decoded frame's index
> > and compares is to VPU-specific state values. Directly before this
> > if and else statements the indexes are read (index for decoded and
> > for displayed frame).
> >
> > Now what is saved in ctx->display_idx is an older value at this point!
>
> Yes. The rotator that copies out the decoded frame runs in parallel 
> with
> the decoding of the next frame. So the decoder signals with display_idx
> which decoded frame should be presented next, but it is only copied out
> into the vb2 buffer during the following run. The same happens with the
> VDOA, but manually, in prepare_decode.
>
> That means that at this point display_idx is the index of the 
> previously
> decoded internal frame that should be presented next, and ctx-
> > display_idx is the index of the internal frame that was just copied
> into the externally visible vb2 buffer.
>
> The logic reads someting like this:
>
> 	if (no frame was decoded) {
> 		if (a frame will be copied out next time) {
> 			adapt sequence number offset;
> 		} else if (no frame was copied out this time) {
> 			hold until further input;
> 		}
> 	}
>
> Basically, it will just wait one more run until it stops the stream,
> assuming that there is really nothing useful in the bitstream
> ringbuffer.
>
> > During these index checks, the current values apply, so fix this by
> > taking display_idx instead of ctx->display_idx.
>
> display_idx is already checked later in the same function.
> According to the i.MX6 VPU API document, it can be -2 (never seen) or 
> -3
> during sequence start (if there is frame reordering, depending on
> whether decoder skip is enabled), and I think I've seen -3 as prescan
> failure on i.MX5. -1 means EOS according to that document, that's why 
> we
> always hold in that case.
>
> > ctx->display_idx is updated later in the same function.
> >
> > Signed-off-by: Martin Kepplinger <martink@xxxxxxxxx>
> > ---
> >
> > Please review this thoroughly, but in case I am wrong here, this is
> > at least very strange to read and *should* be accompanied with a
> > comment about what's going on with that index value!
>
> Maybe it would be less confusing to move this into the display_idx
> checks below, given that we already hold unconditionally
> on display_idx == -1.
>
> > I don't think it matter that much here because at least playing h264
> > worked before and works with this change, but I've tested it anyways.
>
> I think this shouldn't happen at all if you feed it a well formed h.264
> stream. But if you have a skip because of broken data while there is
> still no display frame at the beginning of the stream or after an IDR,
> this might be hit.

Right. Let's leave it this way. In case of real changes, one can think 
about
cleanup.

Thanks for clarification and helping to understand this thing! I 
appreciate it.

                                 martin