Motown: ffmpeg implementations of framework 'parts'

services/media/framework_ffmpeg/ffmpeg_decoder_base.cc

Index: services/media/framework_ffmpeg/ffmpeg_decoder_base.cc
diff --git a/services/media/framework_ffmpeg/ffmpeg_decoder_base.cc b/services/media/framework_ffmpeg/ffmpeg_decoder_base.cc
new file mode 100644
index 0000000000000000000000000000000000000000..517cc7a2f2189e3a6a3c98556bcda911e7877fa4
--- /dev/null
+++ b/services/media/framework_ffmpeg/ffmpeg_decoder_base.cc
@@ -0,0 +1,106 @@
+// 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_decoder_base.h"
+#include "services/media/framework_ffmpeg/ffmpeg_type_converters.h"
+
+namespace mojo {
+namespace media {
+
+FfmpegDecoderBase::FfmpegDecoderBase(AVCodecContext *av_codec_context) :
+    av_codec_context_(av_codec_context),
+    av_frame_(av_frame_alloc()),
+    next_presentation_time_(0) {
+  DCHECK(av_codec_context);
+}
+
+FfmpegDecoderBase::~FfmpegDecoderBase() {}
+
+Result FfmpegDecoderBase::Init(const StreamType& stream_type) {
+  return Result::kOk;

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

Since this class is already abstract, why provide an implementation of
Decoder::Init which does nothing but return OK?  Why not just leave it as
abstract and let the derived class implement it?

Turning it around...  If a derived class implements Decode and CreatePacket, but
not Init, then no one is going to create an AVCodecContex and
output_stream_type() (below) is going to crash.

Alternatively, if you implemented Init here to create a dummy AVCodecContext
whose stream type was STREAM_TYPE_NOTHING or something, you could avoid this,
but leaving Init as abstract seems like a simpler choice.

FWIW - strictly speaking, no data members are supposed to be protected (only
methods).  Perhaps..
1) Init(const StreamType&) should remain abstract
2) Init(unique_ptr<AVCodecContext>) should exist, be non-virtual, and be
protected
3) av_codec_context_ should be made private (with a const unique_ptr<>&
av_codec_context accessor?)

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

AVCodecContext is established in the constructor above. Init isn't needed, and
neither subclass implements it. I'd rather remove it from the Decoder
definition.

There should be no protected fields. I got that feedback in a previous CL and
missed it here. Fixing.

+}
+
+std::unique_ptr<StreamType> FfmpegDecoderBase::output_stream_type() {
+  return StreamTypeFromAVCodecContext(av_codec_context_.get());
+}
+
+void FfmpegDecoderBase::Flush() {
+  avcodec_flush_buffers(av_codec_context_.get());

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

avcodec_flush_buffers is not safe against being called with NULL.  Suggest 

DCHECK(av_codec_context_);

before making this call.

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

Done.

+}
+
+bool FfmpegDecoderBase::TransformPacket(
+    const PacketPtr& input,
+    bool new_input,
+    PayloadAllocator* allocator,
+    PacketPtr* output) {
+  DCHECK(input);
+  DCHECK(allocator);
+  DCHECK(output);
+
+  *output = nullptr;
+
+  if (new_input) {
+    PrepareInputPacket(input);
+  }
+
+  bool frame_decoded = false;
+  int input_bytes_used = Decode(allocator, &frame_decoded);

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

ssize_t instead of int?

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

Decode wraps one of two avcodec_decode_* functions, both of which return int.

+  if (input_bytes_used < 0) {
+    // Decode failed.
+    return UnprepareInputPacket(input, output);
+  }
+
+  if (frame_decoded) {
+    *output = PacketFromAvFrame(allocator);
+  }
+
+  av_packet_.size -= input_bytes_used;
+  av_packet_.data += input_bytes_used;

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

DCHECK(av_packet_.size >= input_bytes_used) and runtime limit input_bytes_used
to be no larger than av_packet_.size

Alternatively; if the decoder claims to have used more bytes than the packet
allows, perhaps the decode should be declared to be a failure.

Right now, there is a risk of underflowing av_packet_.size and leaving
av_packet_.data as a bad pointer.  This opens the possibility for the check
below to decide that we are not done with the ES packet yet, and to continue
processing it, but with a bad size and data pointer next time around.

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

Added a CHECK

+
+  if (av_packet_.size != 0 || (input->end_of_stream() && frame_decoded)) {
+    // The input packet is only partially decoded, or it's an end-of-stream
+    // packet and we're still draining. Let the caller know we want to see the
+    // input packet again.
+    return false;
+  }
+
+  // Used up the whole input packet, and, if we were draining, we're done with
+  // that too.
+  return UnprepareInputPacket(input, output);
+}
+
+void FfmpegDecoderBase::PrepareInputPacket(const PacketPtr& input) {
+  av_init_packet(&av_packet_);
+  av_packet_.data = reinterpret_cast<uint8_t*>(input->payload());
+  av_packet_.size = input->size();
+}
+
+bool FfmpegDecoderBase::UnprepareInputPacket(
+    const PacketPtr& input,
+    PacketPtr* output) {
+  if (input->end_of_stream()) {
+    // Indicate end of stream. This happens when we're draining for the last
+    // time, so there should be no output packet yet.
+    DCHECK(*output == nullptr);
+    *output = Packet::CreateEndOfStream(next_presentation_time_);

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

what is the purpose of putting a PTS on an empty EOS packet?  IOW - why not just
leave it as NOPTS?

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

Design discussion.

+  }
+
+  av_packet_.size = 0;
+  av_packet_.data = nullptr;
+
+  return true;
+}
+
+PacketPtr FfmpegDecoderBase::PacketFromAvFrame(PayloadAllocator* allocator) {
+  DCHECK(allocator);
+
+  PacketPtr packet = CreateOutputPacket(allocator);
+  av_frame_unref(av_frame_.get());
+  next_presentation_time_ = packet->presentation_time() + packet->duration();

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

either presentation_time or duration may be undefined here (ffmpeg uses the
constant AV_NOPTS_VALUE).  If we remove next_presentation_time, we don't need to
worry about this, but if we want to keep it in order to put a PTS on generated
empty EOS frames, then we need to handle these cases (and probably just put
NOPTS into next_presentation_time_)

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

I've moved the next_presentation_time_ into the subclasses, so we won't have a
dependency on the doomed duration field.

+  return packet;
+}
+
+} // namespace media
+} // namespace mojo