Motown: Improvements to packet definition

services/media/framework/packet.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/packet.h"
 #include "services/media/framework/payload_allocator.h"
 
 namespace mojo {
 namespace media {
 
+Packet::Packet(
+    int64_t pts,
+    bool end_of_stream,
+    size_t size,
+    void* payload) :
+    pts_(pts),
+    end_of_stream_(end_of_stream),
+    size_(size),
+    payload_(payload) {
+  DCHECK((size == 0) == (payload == nullptr));
+}
+
 class PacketImpl : public Packet {
  public:
   PacketImpl(
-      int64_t presentation_time,
-      uint64_t duration,
+      int64_t pts,
       bool end_of_stream,
       size_t size,
       void* payload,
       PayloadAllocator* allocator) :
-      presentation_time_(presentation_time),
-      duration_(duration),
-      end_of_stream_(end_of_stream),
-      size_(size),
-      payload_(payload),
-      allocator_(allocator) {
-    DCHECK((size == 0) == (payload == nullptr));
-  }
+      Packet(pts, end_of_stream, size, payload),
+      allocator_(allocator) {}
 
+ protected:
   ~PacketImpl() override {};
 
-  int64_t presentation_time() const override { return presentation_time_; }
-
-  uint64_t duration() const override { return duration_; }
-
-  bool end_of_stream() const override { return end_of_stream_; }
-
-  size_t size() const override { return size_; }
-
-  void* payload() const override { return payload_; }
-
- protected:
   void Release() override {
-    if (payload_ != nullptr && allocator_ != nullptr) {
-      DCHECK(allocator_);
-      allocator_->ReleasePayloadBuffer(size_, payload_);
+    if (payload() != nullptr && allocator_ != nullptr) {

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

we lost the DCHECK(allocator_) here.

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

It seemed unnecessary - the previous line checks that it's not null.

[johngro, 2016/03/21 22:56:05]

fair enough.  What is the intended behavior if payload is non-null and allocator
is nullptr?  I had assumed that this was a mistake and that the intention was to
say something like

if (playload) {
  assert(allocator);
  allocator->release(payload);
}

[dalesat, 2016/03/22 00:06:56]

The code is correct but unclear. Added a comment to explain.

+      allocator_->ReleasePayloadBuffer(size(), payload());
     }
     delete this;
   }
 
  private:
-  int64_t presentation_time_;
-  uint64_t duration_;
-  bool end_of_stream_;
-  size_t size_;
-  void* payload_;
   PayloadAllocator* allocator_;
 };
 
 // static
 PacketPtr Packet::Create(
-    int64_t presentation_time,
-    uint64_t duration,
+    int64_t pts,
     bool end_of_stream,
     size_t size,
     void* payload,
     PayloadAllocator* allocator) {
   DCHECK(payload == nullptr || allocator != nullptr);
   return PacketPtr(new PacketImpl(
-      presentation_time,
-      duration,
+      pts,
       end_of_stream,
       size,
       payload,
       allocator));
 }
 
 // static
 PacketPtr Packet::CreateNoAllocator(
-    int64_t presentation_time,
-    uint64_t duration,
+    int64_t pts,
     bool end_of_stream,
     size_t size,
     void* payload) {
   return PacketPtr(new PacketImpl(
-      presentation_time,
-      duration,
+      pts,
       end_of_stream,
       size,
       payload,
       nullptr));
 }
 
 // static
-PacketPtr Packet::CreateEndOfStream(int64_t presentation_time) {
+PacketPtr Packet::CreateEndOfStream(int64_t pts) {
   return PacketPtr(new PacketImpl(
-      presentation_time,
-      0, // duration
+      pts,
       true, // end_of_stream
       0, // size
       nullptr, // payload
       nullptr)); // allocator
 }
 
 }  // namespace media
 }  // namespace mojo