| Message ID | 20171117143010.501-1-martink@posteo.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 */
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(-)