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 <[email protected]>
>> ---
>>
>> 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
From 1584774036871715493@xxx Wed Nov 22 13:44:56 +0000 2017
X-GM-THRID: 1584342051086866494
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread