Motown: ffmpeg implementations of framework 'parts'

services/media/framework_ffmpeg/ffmpeg_demux.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 <map>
+
+#include "base/logging.h"
+#include "services/media/framework/safe_clone.h"
+#include "services/media/framework_ffmpeg/ffmpeg_demux.h"
+#include "services/media/framework_ffmpeg/ffmpeg_io.h"
+#include "services/media/framework_ffmpeg/ffmpeg_type_converters.h"
+
+namespace mojo {
+namespace media {
+
+class FfmpegDemuxImpl : public FfmpegDemux {
+ public:
+  FfmpegDemuxImpl();
+
+  ~FfmpegDemuxImpl() override;
+
+  // Demux implementation.
+  Result Init(std::shared_ptr<Reader> reader) override;
+
+  std::unique_ptr<Metadata> metadata() const override;
+
+  const std::vector<DemuxStream*>& streams() const override;
+
+  // MultistreamSource implementation.
+  size_t stream_count() const override;
+
+  PacketPtr PullPacket(size_t* stream_index_out) override;
+
+ private:
+  class FfmpegDemuxStream : public DemuxStream {
+   public:
+    FfmpegDemuxStream(AVFormatContext* format_context, size_t index);
+
+    ~FfmpegDemuxStream() override;
+
+    // Demux::DemuxStream implementation.
+    size_t index() const override;
+
+    std::unique_ptr<StreamType> stream_type() const override;
+
+   private:
+    AVStream* stream_;
+    size_t index_;
+    std::unique_ptr<StreamType> stream_type_;
+  };
+
+  // Specialized packet implementation.
+  class DemuxPacket : public Packet {
+   public:
+     ~DemuxPacket() override {

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

Packet's destructor is protected, DemuxPacket's should be as well.

Also; since Packets are created using Packet::Create* which each return a
PacketPtr (which is a unique_ptr with a custom deleter), the Packet constructor
(as well as the constructor's of it's derived classes) should probably be
protected as well.

All of this said, see comments attached to Release below

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

Made the destructor protected. Packet::Create creates packets with the standard
implementation, not DemuxPacket.

+       av_free_packet(&av_packet);

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

what happens if the av_packet was never allocated by ffmpeg?

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

Not sure what you mean. This packet implementation is specifically for wrapping
av_packet.

[johngro, 2016/03/01 22:07:26]

yes, but nothing in this class made certain that av_packet_ had been properly
initialized.  Before, if an instance of the class was created then immediately
destroyed, av_free_packet was going to get called with an un-initialized
instance of AVPacket.

This was the essence of my comment in ffmpeg_demux.cc where you had written

FfmpegDemuxImpl::DemuxPacket* demux_packet =
    new FfmpegDemuxImpl::DemuxPacket();

    av_init_packet(&demux_packet->av_packet);

and I asked you to put the init call in the constructor.

Now that you have, we are certain that the av_packet_ member must have been
properly initialized before the destructor runs, so the problem goes away.

[dalesat, 2016/03/02 18:35:36]

Acknowledged.

+     }
+
+    // Packet implementation.
+    int64_t presentation_time() const override { return av_packet.pts; };
+
+    uint64_t duration() const override { return av_packet.duration; };
+
+    bool end_of_stream() const override { return false; }

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

if PTS, duration and EOS are core pieces of metadata, consider leaving them down
in the Packet level instead of putting them into a custom FFmpeg DemuxPacket
type.

Also, the NOPTS sentinel value used by FFmpeg is different from one that we have
defined.  We should either align our values, or we need to introduce a place to
perform a translation.  These virtual methods might be a place to do such a
thing, but I think that it would be better to do so at creation time, instead of
every time the value is queried.

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

We already have TODOs to revisit the packet definition, including getting rid of
the custom accessors.

+
+    size_t size() const override { return size_t(av_packet.size); }
+
+    void* payload() const override {
+      return reinterpret_cast<void*>(av_packet.data);
+    }
+
+    AVPacket av_packet;

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

this should be private and named av_packet_

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

Done.

+
+   protected:
+    void Release() override { delete this; }

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

so, Packet instances seem to be managed using a std::unique_ptr.  Release is
getting called from the unique_ptr's deleter, which (in this implementation)
calls delete, which then uses the ffmpeg free function to free av_packet.

So, why do we need Release?  IOW - why can't we...

1) Expose only unique_ptrs by using the Create patterns and burying the
constructor.
2) Make the destructor public (so unique_ptr can access it)
3) Implement all custom destruction behavior in the destructor (as is done here)

Right now, my concern is that something could (in theory) call Release while the
unique_ptr is still in scope leaving us with a dead pointer referenced by the
PacketPtr.

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

We already have TODOs to revisit this. I took this approach so we could have a
lookaside of Packet instances.

+  };
+
+  struct AVFormatContextDeleter {
+    inline void operator()(AVFormatContext* ptr) const {
+      avformat_free_context(ptr);
+    }
+  };
+
+  struct AVIOContextDeleter {
+    inline void operator()(AVIOContext* ptr) const {
+      FreeAvioContext(ptr);
+    }
+  };
+
+  // Produces an end-of-stream packet for next_stream_to_end_.
+  PacketPtr PullEndOfStreamPacket(size_t* stream_index_out);
+
+  // Copies metadata from the specified source into map.
+  void CopyMetadata(
+      AVDictionary* source,
+      std::map<std::string, std::string>& map);
+
+  std::shared_ptr<Reader> reader_;
+  std::unique_ptr<AVFormatContext, AVFormatContextDeleter> format_context_;
+  std::unique_ptr<AVIOContext, AVIOContextDeleter> io_context_;
+  std::vector<DemuxStream*> streams_;
+  std::unique_ptr<Metadata> metadata_;
+  int64_t next_presentation_time_;
+  int next_stream_to_end_; // -1 means don't end. streams_.size() means stop.
+};
+
+// static
+std::shared_ptr<Demux> FfmpegDemux::Create() {
+  return std::shared_ptr<Demux>(new FfmpegDemuxImpl());
+}
+
+FfmpegDemuxImpl::FfmpegDemuxImpl() : next_stream_to_end_(-1) {}
+
+FfmpegDemuxImpl::~FfmpegDemuxImpl() {}
+
+Result FfmpegDemuxImpl::Init(std::shared_ptr<Reader> reader) {
+  static const uint64_t kNanosecondsPerMicrosecond = 1000;

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

constexpr

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

Done.

+
+  reader_ = reader;
+
+  AVFormatContext* format_context = avformat_alloc_context();
+
+  io_context_.reset(CreateAvioContext(reader.get()));
+  if (!io_context_) {
+    LOG(ERROR) << "CreateAvioContext failed (allocation failure)";

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

this leaks format_context

I made a similar comment to what follows in the Demux code, but...

Consider extracting management of FFmpeg's unmanged pointer types into some
combination of utility class/namespace.  If you pull stuff like the
AVFormatContextDeleter out of here an put it into the utility region of code,
then you can restrict things to just manipulating the unique pointers, and these
leak hazards should go away.  As part of this, you would do things like bury all
calls FFmpeg's alloc and free functions (the would only ever be invoked from the
utility layer), and this code would become more like

AVFormatContextUPtr format_context = utils::FFmpegAllocAVFormatContext();

// do stuff which might fail and automatically clean up format_context

format_context_ = std::move(format_context);  // explicit ownership transfer


The same potentially hazardous pattern exists here for io_context_, although the
same issue does not as the io_context instance is stored in an io_context
unique_ptr from the get-go.

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

Moved this allocation down to where it belongs. Added a comment proposing the
utility class.

+    return Result::kInternalError;
+  }
+
+  format_context->flags |= AVFMT_FLAG_CUSTOM_IO | AVFMT_FLAG_FAST_SEEK;
+  format_context->pb = io_context_.get();
+
+  int r = avformat_open_input(&format_context, nullptr, nullptr, nullptr);

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

These two operations (open_input and find_stream_info) are blocking operations
in the land of FFmpeg.  Additionally, they have a relatively high probability of
both taking a significant amount of time and eventually failing.

While I understand that shortcuts were taken in the name of producing a demo on
time, these hazards should really be documented with a TODO in the code, and a
bug filed as soon as the code is checked in.

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

Done.

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

Done.

+  format_context_.reset(format_context);
+  if (r < 0) {
+    return Result::kInternalError;
+  }
+
+  r = avformat_find_stream_info(format_context_.get(), nullptr);
+  if (r < 0) {
+    LOG(ERROR) << "avformat_find_stream_info failed, result " << r;
+    return Result::kInternalError;
+  }
+
+  std::map<std::string, std::string> metadata_map;
+
+  CopyMetadata(format_context_->metadata, metadata_map);
+  for (uint i = 0; i < format_context_->nb_streams; i++) {
+    streams_.push_back(new FfmpegDemuxStream(format_context_.get(), i));
+    CopyMetadata(format_context_->streams[i]->metadata, metadata_map);
+  }
+
+  metadata_ = Metadata::Create(
+      format_context_->duration * kNanosecondsPerMicrosecond,
+      metadata_map["TITLE"],
+      metadata_map["ARTIST"],
+      metadata_map["ALBUM"],
+      metadata_map["PUBLISHER"],
+      metadata_map["GENRE"],
+      metadata_map["COMPOSER"]);
+
+  return Result::kOk;
+}
+
+std::unique_ptr<Metadata> FfmpegDemuxImpl::metadata() const {
+  return SafeClone(metadata_);
+}
+
+const std::vector<Demux::DemuxStream*>& FfmpegDemuxImpl::streams() const {
+  return streams_;
+}
+
+size_t FfmpegDemuxImpl::stream_count() const {
+  if (!format_context_) {
+    return 0;
+  }
+  return format_context_->nb_streams;
+}
+
+PacketPtr FfmpegDemuxImpl::PullPacket(size_t* stream_index_out) {
+  DCHECK(stream_index_out);
+
+  if (next_stream_to_end_ != -1) {
+    // We're producing end-of-stream packets for all the streams.
+    return PullEndOfStreamPacket(stream_index_out);
+  }
+
+  FfmpegDemuxImpl::DemuxPacket* demux_packet =
+      new FfmpegDemuxImpl::DemuxPacket();
+
+  av_init_packet(&demux_packet->av_packet);

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

FfmpegDemuxImpl::DemuxPacket owns the call to av_free_packet, it should own the
call to av_init_packet as well.

IOW - no matter what happens, it should be impossible to create an instance of a
FfmpegDemuxImpl::DemuxPacket with an uninitialized av_packet.

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

Done.

+  demux_packet->av_packet.data = nullptr;
+  demux_packet->av_packet.size = 0;

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

access to av_packet should exposed using an accessor (AVPacket& av_packet() {
return av_packet_; } instead of by making the variable public.

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

Done.

+
+  if (av_read_frame(format_context_.get(), &demux_packet->av_packet) < 0) {
+    // End of stream. Start producing end-of-stream packets for all the streams.
+    delete demux_packet;
+    next_stream_to_end_ = 0;
+    return PullEndOfStreamPacket(stream_index_out);
+  }
+
+  *stream_index_out =
+      static_cast<size_t>(demux_packet->av_packet.stream_index);
+  next_presentation_time_ =
+      demux_packet->presentation_time() + demux_packet->duration();

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

either PTS or duration may be AV_NOPTS_VALUE and need to be handled
appropriately.  See earlier comment.

In general, it would be nice to just eliminate the need for
next_presentation_time_.  Right now, it seems to exist only for the purpose of
putting a timestamp on EOS payloads.

Finally, the demux seems to be putting timestamps on generated EOS payloads, it
does not seem like there should be a need to either generate or timestamp EOS
payloads at the decoder level.

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

Acknowledged.

[johngro, 2016/03/01 22:07:26]

Great, if you agree that this can be an issue, but you are not going to fix it
in this go-around, can I please get a TODO so this does not get forgotten?

[dalesat, 2016/03/02 18:35:36]

Done.

+
+  return PacketPtr(demux_packet);
+}
+
+PacketPtr FfmpegDemuxImpl::PullEndOfStreamPacket(size_t* stream_index_out) {
+  DCHECK(next_stream_to_end_ >= 0);
+
+  if (static_cast<std::size_t>(next_stream_to_end_) >= streams_.size()) {
+    NOTREACHED() << "PullPacket called after all streams have ended";
+    return nullptr;
+  }
+
+  *stream_index_out = next_stream_to_end_++;
+  return Packet::CreateEndOfStream(next_presentation_time_);
+}
+
+void FfmpegDemuxImpl::CopyMetadata(
+    AVDictionary* source,
+    std::map<std::string, std::string>& map) {
+  if (source == nullptr) {
+    return;
+  }
+
+  for (AVDictionaryEntry *entry =
+      av_dict_get(source, "", nullptr, AV_DICT_IGNORE_SUFFIX);
+      entry != nullptr;
+      entry = av_dict_get(source, "", entry, AV_DICT_IGNORE_SUFFIX)) {
+    if (map.find(entry->key) == map.end()) {
+      map.emplace(entry->key, entry->value);
+    }
+  }
+}
+
+FfmpegDemuxImpl::FfmpegDemuxStream::FfmpegDemuxStream(
+    AVFormatContext* format_context,

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

Ownership of format_context is not being transferred here, nor is it being
modified, nor may it be NULL.

This should probably be a "const AVFormatContext&" instead of a raw pointer.  If
null-ness is permitted in this system, then it should be a "const
std::unique_ptr<AVFormatContext>&" and a either DCHECK should be run before
accessing the pointer, or proper runtime behavior should be implemented.

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

Done.

+    size_t index) :
+    stream_(format_context->streams[index]), index_(index) {
+  stream_type_ = StreamTypeFromAVCodecContext(stream_->codec);
+}
+
+FfmpegDemuxImpl::FfmpegDemuxStream::~FfmpegDemuxStream() {}
+
+size_t FfmpegDemuxImpl::FfmpegDemuxStream::index() const {
+  return index_;
+}
+
+std::unique_ptr<StreamType> FfmpegDemuxImpl::FfmpegDemuxStream::stream_type()
+    const {
+  return SafeClone(stream_type_);
+}
+
+} // namespace media
+} // namespace mojo