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

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

[Martin Kepplinger]

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!
During these index checks, the current values apply, so fix this by
taking display_idx instead of ctx->display_idx.

ctx->display_idx is updated later in the same function.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---

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!

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.

thanks

                               martin


 drivers/media/platform/coda/coda-bit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index bfc4ecf6f068..fe38527a90e2 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -2089,7 +2089,7 @@ static void coda_finish_decode(struct coda_ctx *ctx)
 		/* no frame was decoded, but we might have a display frame */
 		if (display_idx >= 0 && display_idx < ctx->num_internal_frames)
 			ctx->sequence_offset++;
-		else if (ctx->display_idx < 0)
+		else if (display_idx < 0)
 			ctx->hold = true;
 	} else if (decoded_idx == -2) {
 		/* no frame was decoded, we still return remaining buffers */
-- 
2.11.0

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@posteo.de>
> ---
> 
> 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

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@posteo.de>
>> ---
>> 
>> 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