Motown in-proc streaming framework used to implement media services.

services/media/framework/packet.h

Index: services/media/framework/packet.h
diff --git a/services/media/framework/packet.h b/services/media/framework/packet.h
new file mode 100644
index 0000000000000000000000000000000000000000..e3843b8f50dd667a895f5caf26cfa21592910075
--- /dev/null
+++ b/services/media/framework/packet.h
@@ -0,0 +1,71 @@
+// 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.
+
+#ifndef SERVICES_MEDIA_FRAMEWORK_PACKET_H_
+#define SERVICES_MEDIA_FRAMEWORK_PACKET_H_
+
+#include <memory>
+
+#include "base/logging.h"
+#include "services/media/framework/allocator.h"
+#include "services/media/framework/ptr.h"
+
+namespace mojo {
+namespace media {
+
+class Packet;
+
+// Used for PacketPtr.
+struct PacketDeleter {
+  void operator()(Packet* ptr) const;
+};
+
+// Unique pointer for packets.
+typedef UniquePtr<Packet, PacketDeleter> PacketPtr;
+
+// Media packet abstract base class. Subclasses may be defined as needed.
+// Packet::Create and Packet::CreateEndOfStream use an implementation with
+// no special behavior.
+class Packet {
+ public:
+  // Creates a packet. If size is 0, payload must be nullptr and vice-versa.
+  // If payload is not nullptr, an allocator must be provided.
+  static PacketPtr Create(
+      int64_t presentation_time,
+      uint64_t duration,
+      bool end_of_stream,

[johngro, 2016/01/26 01:32:39]

<design_point note="No code change requested>
PTS, duration and EOS are all pieces of metadata for a Packet (payload, media
access unit, whatever), but do not represent the sum total of per-packet
metadata we may wish to express.  Other pieces of metadata which can be useful
for some (but not all) packets include things like...

1) Random-access-point, drop-able flags for codecs with interpacket
dependencies.
2) Colorspaces, packing format (subsampling, plane construction, etc..) for raw
frames of video.
3) Mandated downmix matricies (or references to them) for decoded audio.
4) Language codes for subpicture packets.

and so on.  It may be good to consider decoupling the metadata from packet and
having the packet just hold onto a reference to the metadata.
</design_point>

Stepping back from the design question, I could be convinced that the one piece
of universal metadata which is useful to all packets is the presentation
timestamp.  Even though it sometimes takes on the special value "Unknown", it is
so commonly accessed that I can understand hoisting it up to the class Packet
level of things and using a sentinel value for "Unknown".  In this case, why are
we accessing the PTS via a virtual function?  Wouldn't it be better to make the
timestamp a private member of the base Packet class (accessible using an inline
public accessor) and require its definition by the subclass using an explicit
protected constructor?

[dalesat, 2016/01/26 21:17:51]

Ack comments about more metadata.

Are you concerned about performance issues with PTS? The entire thing is
abstract, because some implementations are backed by structures defined by e.g.
ffmpeg (AVFrame and AVPacket are examples).

[johngro, 2016/01/27 22:35:22]

mildly; its one of those things which feel like a coin flip now, but later who
knows.  If the timestamp on a packet once created is immutable (a principal I
think we both agree is a worthwhile goal), then it should be simple to have the
protected constructor of Packet demand that a PTS be provided at construction
time.  I could be then accessed with a inline const accessor for pretty much no
cost.

If the underlying packet is going to undergo transformations which alter the
PTS, and this is going to happen inside the context of something like an ffmpeg
operation on an encapsulated ffmpeg structure, then this technique will not work
without a bunch of manual labor which will be hard to maintain.

Finally, if we are directly accessing the internal ffmpeg representation of an
encapsulated PTS, then we are being implicitly forced to use the same sentinel
value for no PTS that ffmpeg uses.  If we are operating under the non-volatile
PTS assumption given above, and choose to cache the PTS locally in this base
class for inline access, then we can map from their value to our value as needed
at construction time.  If not, then we need to be sure that the values are the
same.  If they are already, then good.  If not, then we can change ours to match
theirs provided there are no design concerns about their choice (I would not use
0 or -1 as my sentinel; either 0x8000... or 0x7FFF... seem like better choices).
 Either way, we should have a static_assert somewhere in the
FfmpegBackedPacketImpl code which asserts that their value matches our value
(IIRC, they use a #def called NO_PTS)

[dalesat, 2016/01/28 18:49:15]

Looking at my ffmpeg code, it appears that demuxes are always giving us
timestamps (so far, at least). The decoders don't, but they're using the vanilla
Packet implementation. I add presentation time myself, in that case.

[johngro, 2016/02/01 22:38:16]

so I went ahead and checked in the current ffmpeg source.  The constant is
called AV_NOPTS_VALUE.  It is currently set to 0x80000.... (the most negative
int64_t).  We have our version defined in media_pipe.mojom as 0x7ffff... (the
most positive int64_t).  I still feel that if we are going to provide direct
access to the value that we should make certain to have a static assert that
both of these line up.

FWIW - an example of a demuxer which routinely produces this value for its
packets is the MPEG-2 PS/PES demuxer.  Both the DTS and the PTS are initialized
to NOPTS; if there is no timestamps at the PS/PES level (common for non-random
access points in MPEG-2 encapsulated content), these will remain this way. 
libavformat/avformat.h goes into some detail about the use of the sentinel value
(which fields it may be used for, what it means when it is used that way,
etc...)

I'm fine with leaving this as a TODO or a tracking bug for now.

[dalesat, 2016/02/01 23:01:28]

Acknowledged.

+      uint64_t size,
+      void* payload,
+      Allocator* allocator);

[johngro, 2016/01/26 01:32:39]

Looking at the implementation (next file), it seems like we have 3 things here
(size, payload and allocator) which all belong together.  The Packet class is
going to hold a pointer which came from an allocator and refers to a particular
size allocation.  When it is time to release, the Packet is going to call the
allocator's free equivalent.

If any of these things are mismatched, Bad Things could happen.  We don't want
to return a payload to the incorrect allocator, we don't want the size to be off
when we do so, and so on.

Would it be a good idea to introduce the concept of an Allocator::Buffer?  All
Allocator::Buffers would have a void* member which points to the buffer memory
itself.  For a default allocator (based on malloc/free or new/delete), this
would be all that is needed.  For other Allocator implementations, their Buffer
implementation could contain a reference to the allocator which generated the
Buffer as well as any bookkeeping (like size) which would be needed by the
allocator on a per-packet basis to properly free the buffer.

Alternatively, if there is bookkeeping which is useful to external users of the
buffer (size, for example), it could be pushed down to the base Buffer
implementation and accessed with an inline accessor (like payload).

Finally, if allocators create std::unique_ptr<Allocator::Buffer>s, and
Allocator::Buffer enforces move-semantics, then we have an extensible package of
all the things we need to return the buffer to its owner allocator as well as
implicit lifetime management, and we can pass just the unique_ptr to the buffer
to this class instead of needing to pass all three members which need to stick
together in order to be coherent.

[dalesat, 2016/01/26 21:17:51]

That's a worthy design idea. Can we save it for another CL?

[johngro, 2016/01/27 22:35:22]

sure, if you are serious about implementing it, you may want to file a bug to
track the idea so that we don't forget (this is what I ended up doing for all of
Jeff's design change requests during the AudioServer code review)

[dalesat, 2016/01/28 18:49:15]

Acknowledged.

+
+  // Creates an end-of-stream packet with no payload.
+  static PacketPtr CreateEndOfStream(int64_t presentation_time);
+
+  virtual int64_t presentation_time() const = 0;
+
+  virtual uint64_t duration() const = 0;
+
+  virtual bool end_of_stream() const = 0;
+
+  virtual uint64_t size() const = 0;
+
+  virtual void* payload() const = 0;
+
+ protected:
+  virtual ~Packet() {}
+
+  virtual void release() = 0;
+
+  friend PacketDeleter;
+};
+
+inline void PacketDeleter::operator()(Packet* ptr) const {
+  DCHECK(ptr);
+  ptr->release();
+}
+
+}  // namespace media
+}  // namespace mojo
+
+#endif // SERVICES_MEDIA_FRAMEWORK_PACKET_H_