Motown: ffmpeg implementations of framework 'parts'

services/media/framework_ffmpeg/ffmpeg_audio_decoder.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/logging.h"
+#include "services/media/framework_ffmpeg/ffmpeg_audio_decoder.h"
+
+namespace mojo {
+namespace media {
+
+FfmpegAudioDecoder::FfmpegAudioDecoder(AVCodecContext *av_codec_context) :
+    FfmpegDecoderBase(av_codec_context) {
+  DCHECK(av_codec_context_);
+  av_codec_context_->opaque = this;
+  av_codec_context_->get_buffer2 = AllocateBufferForAvFrame;
+  av_codec_context_->refcounted_frames = 1;
+
+  if (av_sample_fmt_is_planar(av_codec_context->sample_fmt)) {

[johngro, 2016/03/01 18:05:35]

DCHECK(av_codec_context_->channels);

There is a non-zero assumption made for this field in the planar path of
Allocate below.

[dalesat, 2016/03/03 20:41:10]

Done.

+    // Prepare for interleaving.
+    stream_type_ = output_stream_type();
+    lpcm_util_ = LpcmUtil::Create(*stream_type_->lpcm());
+    // Because we'll be copying the output frames when we interleave, we use
+    // the default allocator to make buffers for the non-interelaved frames.
+    // When we interleave, we'll get the output buffer from the provided
+    // allocator.
+    allocator_ = PayloadAllocator::GetDefault();
+  }
+}
+
+FfmpegAudioDecoder::~FfmpegAudioDecoder() {}
+
+int FfmpegAudioDecoder::Decode(
+    PayloadAllocator* allocator,
+    bool* frame_decoded_out) {
+  DCHECK(allocator);
+  DCHECK(frame_decoded_out);
+  DCHECK(av_codec_context_);
+  DCHECK(av_frame_);
+
+  // These get set in AllocateBufferForAvFrame.
+  packet_size_ = 0;
+  packet_buffer_ = nullptr;
+
+  // Use the provided allocator unless we intend to interleave later.
+  if (!lpcm_util_) {
+    allocator_ = allocator;
+  }
+
+  int frame_decoded = 0;
+  int input_bytes_used = avcodec_decode_audio4(
+      av_codec_context_.get(),
+      av_frame_.get(),
+      &frame_decoded,
+      &av_packet_);
+  *frame_decoded_out = frame_decoded != 0;
+
+  // Unless we are interleaving, we're done with this allocator.
+  if (!lpcm_util_) {
+    allocator_ = nullptr;
+  }
+
+  // Make sure allocation occurred as expected.
+  DCHECK(!frame_decoded || packet_size_ != 0);
+  DCHECK(!frame_decoded || packet_buffer_ == av_frame_->data[0]);
+
+  return input_bytes_used;
+}
+
+PacketPtr FfmpegAudioDecoder::CreateOutputPacket(PayloadAllocator* allocator) {
+  DCHECK(allocator);
+  DCHECK(av_frame_);
+
+  int64_t presentation_time = av_frame_->pts;
+  if (presentation_time == AV_NOPTS_VALUE) {
+    presentation_time = next_presentation_time_;

[johngro, 2016/03/01 01:31:38]

I don't think that you should do this.  If the generated frame contains no PTS,
interpolation should be left to the renderer.

The exception to this would be immediately after a discontinuity.  Presuming
that the discontinuity is caused by a seek operation (initial startup being just
a special case of seek), then the container should be producing the initial
timestamp.

In the very special case of a container which contains absolutely no timestamps,
the demux code should simply put an arbitrary timestamp on the first payload and
NOPTS on subsequent payloads.

[dalesat, 2016/03/01 20:43:01]

We should have design discussions like this in another forum.

+  }
+  // TODO(dalesat): Are we sure all decoders use frames as time unit?
+
+  uint64_t payload_size;
+  void *payload_buffer;
+
+  if (lpcm_util_) {
+    // We need to interleave. The non-interleaved frames are in a buffer that
+    // was allocated from allocator_. That buffer will get released later in
+    // ReleaseBufferForAvFrame. We need a new buffer for the interleaved frames,
+    // which we get from the provided allocator.
+    DCHECK(stream_type_);
+    DCHECK(stream_type_->lpcm());
+    payload_size = stream_type_->lpcm()->min_buffer_size(av_frame_->nb_samples);
+    payload_buffer = allocator->AllocatePayloadBuffer(payload_size);
+
+    lpcm_util_->Interleave(
+        av_frame_->data[0],
+        packet_size_,
+        payload_buffer,
+        av_frame_->nb_samples);
+  } else {
+    payload_size = packet_size_;
+    payload_buffer = av_frame_->data[0];

[johngro, 2016/03/01 18:05:35]

So, I think that ffmpeg is going to end up freeing your buffer out from under
you in the interleaved case.  The sequence would be..

1) During decode, ffmpeg calls your AllocateBufferForAvFrame hook
2) A single buffer is allocated using self->allocator_.
2.1) av_frame_->data[0] now points to this buffer.
2.2) the call to av_buffer_create also holds a pointer to the buffer as well as
the hook for ReleaseBufferForAvFrame.
3) After decode, framework calls PacketFromAvFrame.
3.1) PacketFromAvFrame calls CreateOutputPacket (here in the code), and you grab
a pointer to the buffer, but do not detach the buffer from the av_frame_
instance.
3.2) Packet::Create is called and passed a pointer to the buffer.
3.3) PacketFromAvFrame calls av_frame_unref.
3.4) av_frame_unref eventually calls the ReleaseBufferHook which frees the
buffer.
4) The packet is sent down the line.  It is holding a dead pointer.  Eventually
(assuming it has not already crashed) it will double-free this pointer.

[dalesat, 2016/03/03 20:41:10]

This code actually works (ReleaseBufferForAvFrame won't free the buffer because
allocator_ == nullptr), but I'm not happy with it. I wrote it some months ago,
and I had trouble figuring out why it works. I've defined a new class called
AvBufferContext, which I'm using as the opaque context for AVBuffer.
AvBufferContext retains the buffer pointer, buffer size and allocator. Its
Release method releases ownership of the buffer so it won't get released in
ReleaseBufferForAvFrame.

+  }
+
+  return Packet::Create(
+      presentation_time,
+      av_frame_->nb_samples,
+      false, // The base class is responsible for end-of-stream.
+      payload_size,
+      payload_buffer,
+      allocator);
+}
+
+int FfmpegAudioDecoder::AllocateBufferForAvFrame(
+    AVCodecContext* av_codec_context,
+    AVFrame* av_frame,
+    int flags) {
+  // CODEC_CAP_DR1 is required in order to do allocation this way.
+  DCHECK(av_codec_context->codec->capabilities & CODEC_CAP_DR1);
+
+  FfmpegAudioDecoder* self =
+      reinterpret_cast<FfmpegAudioDecoder*>(av_codec_context->opaque);
+  DCHECK(self);
+  DCHECK(self->allocator_);
+  DCHECK(self->packet_size_ == 0) << "multiple allocations per decode";
+
+  AVSampleFormat av_sample_format =
+      static_cast<AVSampleFormat>(av_frame->format);
+
+  int buffer_size = av_samples_get_buffer_size(
+      &av_frame->linesize[0],
+      av_codec_context->channels,
+      av_frame->nb_samples,
+      av_sample_format,
+      FfmpegAudioDecoder::kChannelAlign);
+  if (buffer_size < 0) {

[johngro, 2016/03/01 18:05:35]

<= 0

Also, need to make sure that the rest of the code will produce coherent results
if you shortcut at this point (av_buffer_create is not going to be called if you
bail out now, for example).

That, or make sure the code below will tolerate a 0 buffer size.  Right now,
AllocatePayloadBuffer in the default allocator will not tolerate a request to
allocate 0 bytes.

[dalesat, 2016/03/03 20:41:10]

Opted to tolerate buffer_size == 0

+    return buffer_size;
+  }
+
+  uint8_t* buffer = static_cast<uint8_t*>(
+      self->allocator_->AllocatePayloadBuffer(buffer_size));
+
+  if (!av_sample_fmt_is_planar(av_sample_format)) {
+    // Samples are interleaved. There's just one buffer.
+    av_frame->data[0] = buffer;
+  } else {
+    // Samples are not interleaved. There's one buffer per channel.
+    int channels = av_codec_context->channels;
+    int bytes_per_channel = buffer_size / channels;
+    uint8_t* channel_buffer = buffer;
+
+    if (channels <= AV_NUM_DATA_POINTERS) {
+      // The buffer pointers will fit in av_frame->data.
+      DCHECK_EQ(av_frame->extended_data, av_frame->data);
+      for (int channel = 0; channel < channels; ++channel) {
+        av_frame->data[channel] = channel_buffer;
+        channel_buffer += bytes_per_channel;
+      }
+    } else {
+      // Too many channels for av_frame->data. We have to use
+      // av_frame->extended_data
+      av_frame->extended_data = static_cast<uint8_t**>(
+          av_malloc(channels * sizeof(*av_frame->extended_data)));
+
+      // The first AV_NUM_DATA_POINTERS go in both data and extended_data.
+      int channel = 0;
+      for (; channel < AV_NUM_DATA_POINTERS; ++channel) {
+        av_frame->extended_data[channel] = av_frame->data[channel] =
+            channel_buffer;
+        channel_buffer += bytes_per_channel;
+      }
+
+      // The rest go only in extended_data.
+      for (; channel < channels; ++channel) {
+        av_frame->extended_data[channel] = channel_buffer;
+        channel_buffer += bytes_per_channel;
+      }
+    }
+  }
+
+  av_frame->buf[0] = av_buffer_create(
+      buffer,
+      buffer_size,
+      ReleaseBufferForAvFrame,
+      self,
+      0); // flags
+
+  // We lose the buffer above before CreatePacket gets called, so we save the

[johngro, 2016/03/01 18:05:35]

Why do we lose the buffer?  Isn't the pointer to it stored in av_frame->buf[0]? 
It is not an opaque data structure, you should just be able to use the size/data
members of av_frame->buf[0] instead of needing to stash values in the class.

[dalesat, 2016/03/03 20:41:10]

Done.

+  // size here.
+  self->packet_size_ = buffer_size;
+
+  // This is just to make sure the buffer is used as we intended.
+  self->packet_buffer_ = buffer;
+
+  return 0;
+}
+
+void FfmpegAudioDecoder::ReleaseBufferForAvFrame(
+    void* opaque,
+    uint8_t* buffer) {
+  FfmpegAudioDecoder* self = reinterpret_cast<FfmpegAudioDecoder*>(opaque);
+  if (self->allocator_ != nullptr) {
+    // Either the decoder is releasing this buffer before returning from
+    // avcodec_decode_audio4, or we're interleaving. In either case, we need
+    // to release this buffer, because it won't end up in an output packet.
+    self->allocator_->ReleasePayloadBuffer(self->packet_size_, buffer);
+  }
+}
+
+} // namespace media
+} // namespace mojo