[haiku-commits] Re: haiku: hrev47576 - src/add-ons/media/plugins/ffmpeg
- From: Stephan Aßmus <superstippi@xxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Tue, 29 Jul 2014 12:29:23 +0200
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?
Other related posts:
