Motown: Improvements to packet definition

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/av_packet.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();
@@ skipping 28 matching lines @@
 
    private:
     AVStream* stream_;
     size_t index_;
     std::unique_ptr<StreamType> stream_type_;
   };
 
   // Specialized packet implementation.
   class DemuxPacket : public Packet {
    public:
-    DemuxPacket() {
-      av_init_packet(&av_packet_);
-    }
-
-    // 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; }
-
-    size_t size() const override { return size_t(av_packet_.size); }
-
-    void* payload() const override {
-      return reinterpret_cast<void*>(av_packet_.data);
+    static PacketPtr Create(AvPacketPtr av_packet) {
+      return PacketPtr(new DemuxPacket(std::move(av_packet)));
     }
 
     AVPacket& av_packet() {
-      return av_packet_;
+      return *av_packet_;
     }
 
    protected:
-    ~DemuxPacket() override {
-      av_free_packet(&av_packet_);
-    }
+    ~DemuxPacket() override {}
 
     void Release() override { delete this; }
 
-  private:
-    AVPacket av_packet_;
+   private:
+    DemuxPacket(AvPacketPtr av_packet) :
+        Packet(
+            av_packet->pts,

[johngro, 2016/03/21 19:47:56]

Since this is a place where we we are coming from the FFmpeg world into our
world, we should take care to translate the unknown PTS sentinel value.

IOW, this should be

(av_packet->pts == AV_NOPTS_VALUE) ? kUnknownPts : av_packet->pts),

If we have pegged these to the same value, then we should add a
static_assert(AV_NOPTS_VALUE == kUnknownPts) (Immediately before the constructor
declaration is probably the best place).

Having a DCHECK to make sure that av_packet is non-null before accessing it
would be nice as well, but I am reluctant to ask you to re-write your
initialization list as an in-body initialization.

Finally, a style nit which I don't care about (take it or leave it), we are
supposed to drop the : for initializer lists.

[dalesat, 2016/03/21 22:30:59]

Done.

+            false,
+            static_cast<size_t>(av_packet->size),

[johngro, 2016/03/21 19:47:56]

If this cast is required because av_packet->size is signed, it would be good to
DCHECK(av_packet->size >= 0) somewhere in the constructor body.

[dalesat, 2016/03/21 22:30:59]

Done.

+            av_packet->data),
+        av_packet_(std::move(av_packet)) {}
+
+    AvPacketPtr av_packet_;
   };
 
   struct AVFormatContextDeleter {
     inline void operator()(AVFormatContext* ptr) const {
       avformat_free_context(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_;
   AvioContextPtr io_context_;
   std::vector<DemuxStream*> streams_;
   std::unique_ptr<Metadata> metadata_;
-  int64_t next_presentation_time_;
+  int64_t next_pts_;
   int next_stream_to_end_ = -1; // -1: don't end, streams_.size(): stop.
 };
 
 // static
 std::shared_ptr<Demux> FfmpegDemux::Create() {
   return std::shared_ptr<Demux>(new FfmpegDemuxImpl());
 }
 
 FfmpegDemuxImpl::FfmpegDemuxImpl() {}
 
@@ skipping 73 matching lines @@
 }
 
 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();
+  AvPacketPtr av_packet = AvPacket::Create();
 
-  demux_packet->av_packet().data = nullptr;
-  demux_packet->av_packet().size = 0;
+  av_packet->data = nullptr;
+  av_packet->size = 0;
 
-  if (av_read_frame(format_context_.get(), &demux_packet->av_packet()) < 0) {
+  if (av_read_frame(format_context_.get(), av_packet.get()) < 0) {
     // End of stream. Start producing end-of-stream packets for all the streams.
-    PacketPtr(demux_packet); // Deletes 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);
+      static_cast<size_t>(av_packet->stream_index);
   // TODO(dalesat): What if the packet has no PTS or duration?
-  next_presentation_time_ =
-      demux_packet->presentation_time() + demux_packet->duration();
+  next_pts_ = av_packet->pts + av_packet->duration;
 
-  return PacketPtr(demux_packet);
+  return DemuxPacket::Create(std::move(av_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_);
+  return Packet::CreateEndOfStream(next_pts_);
 }
 
 void FfmpegDemuxImpl::CopyMetadata(
     AVDictionary* source,
     std::map<std::string, std::string>& map) {
   if (source == nullptr) {
     return;
   }
 
   for (AVDictionaryEntry *entry =
@@ skipping 19 matching lines @@
   return index_;
 }
 
 std::unique_ptr<StreamType> FfmpegDemuxImpl::FfmpegDemuxStream::stream_type()
     const {
   return SafeClone(stream_type_);
 }
 
 } // namespace media
 } // namespace mojo