Hi Colin,these changes are highly appreciated. Pointed out some style violations below:
Am 26.07.2014 16:39, schrieb coling@xxxxxx:
+void +AVCodecDecoder::_DeinterlaceAndColorConvertVideoFrame() +{ + int width = fOutputVideoFormat.display.line_width; + int height = fOutputVideoFormat.display.line_count; + AVPicture deinterlacedPicture; + bool useDeinterlacedPicture = false; + + if (fRawDecodedPicture->interlaced_frame) { + AVPicture rawPicture; + rawPicture.data[0] = fRawDecodedPicture->data[0]; + rawPicture.data[1] = fRawDecodedPicture->data[1]; + rawPicture.data[2] = fRawDecodedPicture->data[2]; + rawPicture.data[3] = fRawDecodedPicture->data[3]; + rawPicture.linesize[0] = fRawDecodedPicture->linesize[0]; + rawPicture.linesize[1] = fRawDecodedPicture->linesize[1]; + rawPicture.linesize[2] = fRawDecodedPicture->linesize[2]; + rawPicture.linesize[3] = fRawDecodedPicture->linesize[3]; + + avpicture_alloc(&deinterlacedPicture, + fContext->pix_fmt, width, height); + + if (avpicture_deinterlace(&deinterlacedPicture, &rawPicture, + fContext->pix_fmt, width, height) < 0) { + TRACE("[v] avpicture_deinterlace() - error\n"); + } else + useDeinterlacedPicture = true; + } + + // Some decoders do not set pix_fmt until they have decoded 1 frame +#if USE_SWS_FOR_COLOR_SPACE_CONVERSION + if (fSwsContext == NULL) { + fSwsContext = sws_getContext(fContext->width, fContext->height, + fContext->pix_fmt, fContext->width, fContext->height, + colorspace_to_pixfmt(fOutputVideoFormat.display.format), + SWS_FAST_BILINEAR, NULL, NULL, NULL); + } +#else + if (fFormatConversionFunc == NULL) { + fFormatConversionFunc = resolve_colorspace( + fOutputVideoFormat.display.format, fContext->pix_fmt, + fContext->width, fContext->height); + } +#endif + + fDecodedDataSizeInBytes = avpicture_get_size( + colorspace_to_pixfmt(fOutputVideoFormat.display.format), + fContext->width, fContext->height); + + if (fDecodedData == NULL) + fDecodedData + = static_cast<uint8_t*>(malloc(fDecodedDataSizeInBytes)); + + fPostProcessedDecodedPicture->data[0] = fDecodedData; + fPostProcessedDecodedPicture->linesize[0] + = fOutputVideoFormat.display.bytes_per_row; + +#if USE_SWS_FOR_COLOR_SPACE_CONVERSION + if (fSwsContext != NULL) { +#else + if (fFormatConversionFunc != NULL) { +#endif + if (useDeinterlacedPicture) { + AVFrame deinterlacedFrame; + deinterlacedFrame.data[0] = deinterlacedPicture.data[0]; + deinterlacedFrame.data[1] = deinterlacedPicture.data[1]; + deinterlacedFrame.data[2] = deinterlacedPicture.data[2]; + deinterlacedFrame.data[3] = deinterlacedPicture.data[3]; + deinterlacedFrame.linesize[0] + = deinterlacedPicture.linesize[0]; + deinterlacedFrame.linesize[1] + = deinterlacedPicture.linesize[1]; + deinterlacedFrame.linesize[2] + = deinterlacedPicture.linesize[2]; + deinterlacedFrame.linesize[3] + = deinterlacedPicture.linesize[3]; + +#if USE_SWS_FOR_COLOR_SPACE_CONVERSION + sws_scale(fSwsContext, deinterlacedFrame.data, + deinterlacedFrame.linesize, 0, fContext->height, + fPostProcessedDecodedPicture->data, + fPostProcessedDecodedPicture->linesize); +#else + (*fFormatConversionFunc)(&deinterlacedFrame, + fPostProcessedDecodedPicture, width, height); +#endif + } else { +#if USE_SWS_FOR_COLOR_SPACE_CONVERSION + sws_scale(fSwsContext, fRawDecodedPicture->data, + fRawDecodedPicture->linesize, 0, fContext->height, + fPostProcessedDecodedPicture->data, + fPostProcessedDecodedPicture->linesize); +#else + (*fFormatConversionFunc)(fRawDecodedPicture, + fPostProcessedDecodedPicture, width, height); +#endif + } + } + + if (fRawDecodedPicture->interlaced_frame) + avpicture_free(&deinterlacedPicture); +}
The function body is indented one too many levels (left-over from copy&paste).
diff --git a/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.cpp b/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.cpp index 9db9929..8d52cc5 100644 --- a/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.cpp +++ b/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.cpp @@ -430,6 +430,11 @@ AVCodecDecoder::_NegotiateVideoOutputFormat(media_format* inOutFormat) fContext->extradata = (uint8_t*)fExtraData; fContext->extradata_size = fExtraDataSize; + if (fCodec->capabilities & CODEC_CAP_TRUNCATED) + // Expect and handle video frames to be splitted across consecutive + // data chunks. + fContext->flags |= CODEC_FLAG_TRUNCATED; +
- This is a multi-line if-statement. - The comparison is no boolean expression. Correct would be: if ((fCodec->capabilities & CODEC_CAP_TRUNCATED) != 0) { // Expect and handle video frames to be splitted // across consecutive data chunks. fContext->flags |= CODEC_FLAG_TRUNCATED; }
+ if (fCodecInitDone) { avcodec_flush_buffers(fContext); + av_init_packet(&fTempPacket); + fTempPacket.size = 0; + fTempPacket.data = NULL; + }
Please extract a small "_ResetTempPacket()" method, see below.
- if (fCodec->capabilities & CODEC_CAP_TRUNCATED) + if (fCodec->capabilities & CODEC_CAP_TRUNCATED) { // Expect and handle video frames to be splitted across consecutive // data chunks. fContext->flags |= CODEC_FLAG_TRUNCATED; + }
Ah, you corrected the brackets, but please add the "() != 0".
TRACE(" requested video format 0x%x\n", inOutFormat->u.raw_video.display.format); @@ -516,6 +522,10 @@ AVCodecDecoder::_NegotiateVideoOutputFormat(media_format* inOutFormat) inOutFormat->require_flags = 0; inOutFormat->deny_flags = B_MEDIA_MAUI_UNDEFINED_FLAGS; + av_init_packet(&fTempPacket); + fTempPacket.size = 0; + fTempPacket.data = NULL;
Duplicated-code-alarm, see above.
#ifdef TRACE_AV_CODEC char buffer[1024]; string_for_format(*inOutFormat, buffer, sizeof(buffer)); @@ -700,24 +710,35 @@ AVCodecDecoder::_DecodeVideo(void* outBuffer, int64* outFrameCount, status_t AVCodecDecoder::_DecodeNextVideoFrame() { + assert(fTempPacket.size >= 0);
Wrong assert. Conflicts with first if-check in the while-loop.
bool firstRun = true; while (true) { media_header chunkMediaHeader; - status_t err = GetNextChunk(&fChunkBuffer, &fChunkBufferSize, - &chunkMediaHeader); - if (err != B_OK) { - TRACE("AVCodecDecoder::_DecodeVideo(): error from " - "GetNextChunk(): %s\n", strerror(err)); - return err; - } + + if (fTempPacket.size == 0) { + // Our packet buffer is empty, so fill it now. + status_t getNextChunkStatus = GetNextChunk(&fChunkBuffer,
weird white-space before "="
+ &fChunkBufferSize, &chunkMediaHeader); + if (getNextChunkStatus != B_OK) { + TRACE("AVCodecDecoder::_DecodeNextVideoFrame(): error from " + "GetNextChunk(): %s\n", strerror(err)); + return getNextChunkStatus; + } + + fTempPacket.data = static_cast<uint8_t*>(const_cast<void*>( + fChunkBuffer));
I wonder about the const_cast. Should fChunkBuffer be declared mutable?
- // NOTE: In the FFmpeg code example I've read, the length returned by - // avcodec_decode_video() is completely ignored. Furthermore, the - // packet buffers are supposed to contain complete frames only so we - // don't seem to be required to buffer any packets because not the - // complete packet has been read. - fTempPacket.data = (uint8_t*)fChunkBuffer; - fTempPacket.size = fChunkBufferSize; + // NOTE: In the FFMPEG 0.10.2 code example decoding_encoding.c, the + // length returned by avcodec_decode_video2() is used to update the + // packet buffer size (here it is fTempPacket.size). This way the + // packet buffer is allowed to contain incomplete frames so we are + // required to buffer the packets between different calls to
Nice one!
+/*! Updates relevant fields of the class member fHeader with the properties of + the most recently decoded video frame. + + It is assumed tat this function is called in _DecodeNextVideoFrame() only
Spelling: tat -> that
############################################################################ Commit: 97f5a12f3692b17cb93edeb0f37939ee0290fc7d
@@ -730,6 +748,31 @@ AVCodecDecoder::_DecodeNextVideoFrame() fChunkBuffer)); 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). + // + // FIXME: Research how to establish a meaningful relationship + // between start_time and decoded video frame when the received + // chunk buffer contains partial video frames. Maybe some data + // formats contain time stamps (ake pts / dts fields) that can + // be evaluated by FFMPEG. But as long as I don't have such + // video data to test it, it makes no sense to implement it. + // + // FIXME: Implement tracking start_time of video frames + // originating in data chunks that encode more than one video + // frame at a time. In that case on would increment the + // start_time for each consecutive frame of such a data chunk + // (like it is done for audio frame decoding). But as long as + // I don't have such video data to test it, it makes no sense + // to implement it. + #ifdef LOG_STREAM_TO_FILE if (sDumpedPackets < 100) { sStreamLogFile.Write(fChunkBuffer, fChunkBufferSize); @@ -740,13 +783,6 @@ AVCodecDecoder::_DecodeNextVideoFrame() #endif } - if (firstRun) { - firstRun = false; - - fHeader.start_time = chunkMediaHeader.start_time; - fStartTime = chunkMediaHeader.start_time; - } - #if DO_PROFILING bigtime_t startTime = system_time(); #endif @@ -857,12 +893,13 @@ AVCodecDecoder::_UpdateMediaHeaderForVideoFrame() fHeader.type = B_MEDIA_RAW_VIDEO; fHeader.file_pos = 0; fHeader.orig_size = 0; + fHeader.start_time = fRawDecodedPicture->reordered_opaque; fHeader.u.raw_video.field_gamma = 1.0; fHeader.u.raw_video.field_sequence = fFrame; fHeader.u.raw_video.field_number = 0; fHeader.u.raw_video.pulldown_number = 0; fHeader.u.raw_video.first_active_line = 1; - fHeader.u.raw_video.line_count = fContext->height; + fHeader.u.raw_video.line_count = fRawDecodedPicture->height; TRACE("[v] start_time=%02d:%02d.%02d field_sequence=%lu\n", int((fHeader.start_time / 60000000) % 60),
I don't quiet understand this commit. How exactly is the start time now handled by FFmpeg? It just seems to take a start time which is already established and stores it in "reordered_opaque". This is then somehow transferred by FFmpeg to the decoded picture, so that we match the start times to pictures in case they have been re-ordered. Right? But why is the start-time correct in the first place?