Provide a Chrome-owned buffer to FFmpeg for video decoding, instead of

media/filters/ffmpeg_video_decoder.cc

Index: media/filters/ffmpeg_video_decoder.cc
===================================================================
--- media/filters/ffmpeg_video_decoder.cc	(revision 139175)
+++ media/filters/ffmpeg_video_decoder.cc	(working copy)
@@ -59,6 +59,74 @@
       frame_rate_denominator_(0) {
 }
 
+int FFmpegVideoDecoder::AllocBuffer(VideoFrame **ptr)

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

no doco, no unittest, single call-site ==> inline it into the caller.

+{
+  VideoFrame::Format format = PixelFormatToVideoFormat(codec_context_->pix_fmt);
+  int w = codec_context_->width;
+  int h = codec_context_->height;
+  int ret, strides[4];
+  avcodec_align_dimensions(codec_context_, &w, &h);
+  if ((ret = av_image_check_size(w, h, 0, NULL)) < 0)
+    return ret;
+  if ((ret = av_image_fill_linesizes(strides, codec_context_->pix_fmt, w)) < 0)
+    return ret;
+  VideoFrame *buf = new VideoFrame::VideoFrame(format, codec_context_->width,
+                                               codec_context_->height,
+                                               kNoTimestamp(), kNoTimestamp());
+  if ((ret = buf->AllocateAlignedWithStrides(strides, h)) < 0) {
+    free(buf);
+    return ret;
+  }
+  *ptr = buf;
+  return 0;
+}
+
+int FFmpegVideoDecoder::GetVideoBuffer(AVFrame *frame)
+{
+  VideoFrame *buf;
+  int ret = AllocBuffer(&buf);
+  if (ret < 0)
+    return ret;
+
+  buf->AddRef();

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

AddRef() is almost never the right thing to call.
AllocBuffer should store the result of its "new VideoFrame" in a
scoped_refptr<VideoFrame>, and return that type.

When you need to pass it to C, call .release() on the scoped_refptr.

+  frame->opaque        = buf;

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

This sort of ascii-art alignment isn't adding anything, IMO.

+  frame->type          = FF_BUFFER_TYPE_USER;
+  frame->pkt_pts       = codec_context_->pkt ? codec_context_->pkt->pts : AV_NOPTS_VALUE;
+  frame->width         = codec_context_->width;
+  frame->height        = codec_context_->height;
+  frame->format        = codec_context_->pix_fmt;
+
+  for (int i = 0; i < (buf->format() == media::VideoFrame::RGB32 ? 1 : 3); i++) {

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

I think you're assuming either RGB32 or YV12/YV16.  In that case precede this
for loop with
DCHECK(buf->format() == VideoFrame::YV12 || buf->format() == VideoFrame::YV16 ||
       buf->format() == VideoFrame::RGB32) << buf->format();

       

+    frame->base[i]     =

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

Un-style-ish.  Repeat buf->data(i)

+    frame->data[i]     = buf->data(i);
+    frame->linesize[i] = buf->stride(i);
+  }
+
+  return 0;
+}
+
+static int c_GetVideoBuffer(AVCodecContext *s, AVFrame *frame)

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

the "c_" prefix is un-style-ish.

+{

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

opening braces go on previous line (here and elsewhere)

+  FFmpegVideoDecoder *vd = (FFmpegVideoDecoder *) s->opaque;

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

This isn't used until l.114, so it can be done there.

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

use static_cast<>() instead of c-style casts
(here and elsewhere)

+  if (!(s->flags & CODEC_FLAG_EMU_EDGE)) {

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

When will this be true?
(can you drop the "then" clause?  ditto for l.121 below).

+    return avcodec_default_get_buffer(s, frame);
+  } else {
+    return vd->GetVideoBuffer(frame);
+  }
+}
+
+static void c_ReleaseVideoBuffer(AVCodecContext *s, AVFrame *frame)
+{
+  if (!(s->flags & CODEC_FLAG_EMU_EDGE)) {
+    avcodec_default_release_buffer(s, frame);
+  } else {
+    VideoFrame *buf = (media::VideoFrame *) frame->opaque;
+    buf->Release();
+    memset(frame->data, 0, sizeof(frame->data));

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

why? (also, do you really mean to only set the first 4/8 bytes to 0?)

+    frame->opaque = NULL;
+  }
+}
+
 void FFmpegVideoDecoder::Initialize(const scoped_refptr<DemuxerStream>& stream,
                                     const PipelineStatusCB& status_cb,
                                     const StatisticsCB& statistics_cb) {
@@ -102,6 +170,10 @@
   codec_context_->error_concealment = FF_EC_GUESS_MVS | FF_EC_DEBLOCK;
   codec_context_->err_recognition = AV_EF_CAREFUL;
   codec_context_->thread_count = GetThreadCount(codec_context_->codec_id);
+  codec_context_->opaque = this;
+  codec_context_->flags |= CODEC_FLAG_EMU_EDGE;
+  codec_context_->get_buffer = c_GetVideoBuffer;

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

Depending on ffmpeg dispatches these callbacks, it may be possible to assign to
them the result of a base::Bind() which would allow you to drop the file-static
trampolines.

+  codec_context_->release_buffer = c_ReleaseVideoBuffer;
 
   AVCodec* codec = avcodec_find_decoder(codec_context_->codec_id);
   if (!codec) {
@@ -356,11 +428,15 @@
     return false;
   }
 
-  // We've got a frame! Make sure we have a place to store it.
-  *video_frame = AllocateVideoFrame();
-  if (!(*video_frame)) {
-    LOG(ERROR) << "Failed to allocate video frame";
-    return false;
+  if (!(codec_context_->flags & CODEC_FLAG_EMU_EDGE)) {

[Ami GONE FROM CHROMIUM, 2012/05/26 22:52:34]

Drop this test here and elsewhere?

+    // We've got a frame! Make sure we have a place to store it.
+    *video_frame = AllocateVideoFrame();
+    if (!(*video_frame)) {
+      LOG(ERROR) << "Failed to allocate video frame";
+      return false;
+    }
+  } else {
+    *video_frame = (media::VideoFrame *) av_frame_->opaque;
   }
 
   // Determine timestamp and calculate the duration based on the repeat picture
@@ -381,19 +457,21 @@
   (*video_frame)->SetDuration(
       ConvertFromTimeBase(doubled_time_base, 2 + av_frame_->repeat_pict));
 
-  // Copy the frame data since FFmpeg reuses internal buffers for AVFrame
-  // output, meaning the data is only valid until the next
-  // avcodec_decode_video() call.
-  int y_rows = codec_context_->height;
-  int uv_rows = codec_context_->height;
-  if (codec_context_->pix_fmt == PIX_FMT_YUV420P) {
-    uv_rows /= 2;
+  if (!(codec_context_->flags & CODEC_FLAG_EMU_EDGE)) {
+    // Copy the frame data since FFmpeg reuses internal buffers for AVFrame
+    // output, meaning the data is only valid until the next
+    // avcodec_decode_video() call.
+    int y_rows = codec_context_->height;
+    int uv_rows = codec_context_->height;
+    if (codec_context_->pix_fmt == PIX_FMT_YUV420P) {
+      uv_rows /= 2;
+    }
+
+    CopyYPlane(av_frame_->data[0], av_frame_->linesize[0], y_rows, *video_frame);
+    CopyUPlane(av_frame_->data[1], av_frame_->linesize[1], uv_rows, *video_frame);
+    CopyVPlane(av_frame_->data[2], av_frame_->linesize[2], uv_rows, *video_frame);
   }
 
-  CopyYPlane(av_frame_->data[0], av_frame_->linesize[0], y_rows, *video_frame);
-  CopyUPlane(av_frame_->data[1], av_frame_->linesize[1], uv_rows, *video_frame);
-  CopyVPlane(av_frame_->data[2], av_frame_->linesize[2], uv_rows, *video_frame);
-
   return true;
 }