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

[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.

regards
Philipp