From ed9de7dfca10fd536b288ff84c8af840e1c9111e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20G=C3=BCnther?= Date: Sun, 10 Aug 2014 13:31:45 +0200 Subject: [PATCH] FFMPEG Plugin: Fix video start_time handling - I misinterpreted the semantics of reordered_opaque. I thought it would establish the correct relationship between the start_time returned by GetNextChunk() and the next video frame successfully decoded. But for this to work reordered_opaque expects to be filled with presentation time stamps. A series of presentation time stamps may be jumping back in time due to the presence of B-frames. The decoded frame presentation time stamps series would then be ordered in a monotonically increased way. But actually GetNextChunk() always returns monotonically increasing start times. Mapping this behaviour to FFMPEG's expectations means labeling those start times as decoding time stamps (dts). Though for those start times to be related to the correct decoded video frames you have to assign the start time with the AVPacket containing the data to be decoded. - This commit finally makes DVB video playback working for me with the TV app. Though no audio yet. - The documentation was updated accordingly. --- .../media/plugins/ffmpeg/AVCodecDecoder.cpp | 39 ++++++++----------- .../media/plugins/ffmpeg/AVCodecDecoder.h | 2 +- 2 files changed, 18 insertions(+), 23 deletions(-) diff --git a/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.cpp b/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.cpp index 14925248f6..73bb01f7e9 100644 --- a/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.cpp +++ b/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.cpp @@ -741,7 +741,7 @@ AVCodecDecoder::_DecodeNextVideoFrame() while (true) { status_t loadingChunkStatus - = _LoadNextVideoChunkIfNeededAndUpdateStartTime(); + = _LoadNextVideoChunkIfNeededAndAssignStartTime(); if (loadingChunkStatus == B_LAST_BUFFER_ERROR) return _FlushOneVideoFrameFromDecoderBuffer(); @@ -822,21 +822,21 @@ AVCodecDecoder::_DecodeNextVideoFrame() } -/*! \brief Loads the next video chunk into fVideoChunkBuffer and assigns it to - fTempPacket accordingly only if fTempPacket is empty. Updates fContext - with the start time of the new data chunk. +/*! \brief Loads the next video chunk into fVideoChunkBuffer and assigns it + (including the start time) to fTempPacket accordingly only if + fTempPacket is empty. \returns B_OK - 1. meaning: Next video chunk is loaded and fContext is updated. - 2. meaning: No need to load and update anything. Proceed as usual. + 1. meaning: Next video chunk is loaded. + 2. meaning: No need to load and assign anything. Proceed as usual. \returns B_LAST_BUFFER_ERROR No more video chunks available. - fVideoChunkBuffer, fTempPacket and fContext are left untouched. - \returns Other errors Caller should bail out because fVideoChunkBuffer, - fTempPacket and fContext are in unknown states. Normal operation cannot - be guaranteed. + fVideoChunkBuffer and fTempPacket are left untouched. + \returns Other errors Caller should bail out because fVideoChunkBuffer and + fTempPacket are in unknown states. Normal operation cannot be + guaranteed. */ status_t -AVCodecDecoder::_LoadNextVideoChunkIfNeededAndUpdateStartTime() +AVCodecDecoder::_LoadNextVideoChunkIfNeededAndAssignStartTime() { // TODO: Rename fVideoChunkBuffer to fChunkBuffer, once the audio path is // responsible for releasing the chunk buffer, too. @@ -863,16 +863,11 @@ AVCodecDecoder::_LoadNextVideoChunkIfNeededAndUpdateStartTime() fTempPacket.data = fVideoChunkBuffer; fTempPacket.size = fChunkBufferSize; - - fContext->reordered_opaque = chunkMediaHeader.start_time; - // Let FFMPEG handle the relationship between start_time and - // decoded video frame. - // - // Explanation: - // The received chunk buffer may not contain the next video - // frame to be decoded, due to frame reordering (e.g. MPEG1/2 - // provides encoded video frames in a different order than the - // decoded video frame). + fTempPacket.dts = chunkMediaHeader.start_time; + // Let FFMPEG handle the correct relationship between start_time and + // decoded video frame. By doing so we are simply copying the way how + // it is implemented in ffplay.c + // \see http://git.videolan.org/?p=ffmpeg.git;a=blob;f=ffplay.c;h=09623db374e5289ed20b7cc28c262c4375a8b2e4;hb=9153b33a742c4e2a85ff6230aea0e75f5a8b26c2#l1502 // // FIXME: Research how to establish a meaningful relationship // between start_time and decoded video frame when the received @@ -1039,7 +1034,7 @@ AVCodecDecoder::_UpdateMediaHeaderForVideoFrame() fHeader.type = B_MEDIA_RAW_VIDEO; fHeader.file_pos = 0; fHeader.orig_size = 0; - fHeader.start_time = fRawDecodedPicture->reordered_opaque; + fHeader.start_time = fRawDecodedPicture->pkt_dts; fHeader.size_used = fDecodedDataSizeInBytes; fHeader.u.raw_video.display_line_width = fRawDecodedPicture->width; fHeader.u.raw_video.display_line_count = fRawDecodedPicture->height; diff --git a/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.h b/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.h index 63ee4f9652..516bfc3d1c 100644 --- a/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.h +++ b/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.h @@ -61,7 +61,7 @@ private: media_header* mediaHeader, media_decode_info* info); status_t _DecodeNextVideoFrame(); - status_t _LoadNextVideoChunkIfNeededAndUpdateStartTime(); + status_t _LoadNextVideoChunkIfNeededAndAssignStartTime(); // TODO: Remove the "Video" word once // the audio path is responsible for // freeing the chunk buffer, too.