Motown: Improvements to packet definition

services/media/framework_ffmpeg/ffmpeg_demux.cc

Index: services/media/framework_ffmpeg/ffmpeg_demux.cc
diff --git a/services/media/framework_ffmpeg/ffmpeg_demux.cc b/services/media/framework_ffmpeg/ffmpeg_demux.cc
index b8825883dd3e8758071458b6970fbe9a4d9433d0..38b194daab6fe202254fa9e4b71b94f92829230f 100644
--- a/services/media/framework_ffmpeg/ffmpeg_demux.cc
+++ b/services/media/framework_ffmpeg/ffmpeg_demux.cc
@@ -6,6 +6,7 @@
 
 #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"
@@ -54,36 +55,29 @@ class FfmpegDemuxImpl : public FfmpegDemux {
   // 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 {
@@ -105,7 +99,7 @@ class FfmpegDemuxImpl : public FfmpegDemux {
   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.
 };
 
@@ -199,26 +193,23 @@ PacketPtr FfmpegDemuxImpl::PullPacket(size_t* stream_index_out) {
     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) {
@@ -230,7 +221,7 @@ PacketPtr FfmpegDemuxImpl::PullEndOfStreamPacket(size_t* stream_index_out) {
   }
 
   *stream_index_out = next_stream_to_end_++;
-  return Packet::CreateEndOfStream(next_presentation_time_);
+  return Packet::CreateEndOfStream(next_pts_);
 }
 
 void FfmpegDemuxImpl::CopyMetadata(