Access unit queue for MediaCodecPlayer

media/base/android/access_unit_queue.h

Index: media/base/android/access_unit_queue.h
diff --git a/media/base/android/access_unit_queue.h b/media/base/android/access_unit_queue.h
new file mode 100644
index 0000000000000000000000000000000000000000..23e732260828be044781b0ebebef48ccec953034
--- /dev/null
+++ b/media/base/android/access_unit_queue.h
@@ -0,0 +1,93 @@
+// Copyright 2015 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 MEDIA_BASE_ANDROID_ACCESS_UNIT_QUEUE_H_
+#define MEDIA_BASE_ANDROID_ACCESS_UNIT_QUEUE_H_
+
+#include <list>
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/lock.h"
+#include "media/base/android/demuxer_stream_player_params.h"
+
+namespace media {
+
+// The queue of incoming data for MediaCodecDecoder.
+//
+// The data comes in the form of access units. Each access unit has a type,

[wolenetz, 2015/06/08 21:37:09]

nit: s/, if/. If/

[Tima Vaisburd, 2015/06/09 21:29:21]

Done.

+// if the type is |kConfigChanged| the access unit itself has no data, but
+// is accompanied with DemuxerConfigs.
+// The queue is accessed on the Media thread that puts the incoming data in and

[wolenetz, 2015/06/08 21:37:09]

Are these assumptions checked in the .cc? If not (or if just commented in each
method in the .cc, put those comments here for each method involved, and change
this line to be more like "The queue should be accessed...".

[Tima Vaisburd, 2015/06/09 21:29:21]

No, the assumptions are not checked, and  I changed the comment to "should be".
As far as other methods, however, all public methods except
NumChunksForTesting() are thread safe and can be accessed from any thread. The
comment at the top was more about the general purpose of this class.

[wolenetz, 2015/06/11 19:21:20]

Acknowledged.

+// the Decoder thread that gets the next access unit and eventually removes it
+// from the queue.
+class AccessUnitQueue {
+ public:
+  // Information about the queue state and the access unit at the front.
+  struct Info {
+    // The unit at front. Null if the queue is empty.
+    const AccessUnit* front_unit;
+
+    // Configs for the front unit if it is |kConfigChanged|, null otherwise.
+    const DemuxerConfigs* configs;
+
+    // Number of access units in the queue.
+    int length;
+
+    // Whether the queue contains End Of Stream.

[wolenetz, 2015/06/08 21:37:09]

End of Stream is somewhat ephemeral: a web app could append some data, then mark
the stream as ended, and after that point reopen the stream by various
operations (like appending more data, removing some data, etc.). I'm curious:
what would be the intended behavior of an AccessUnitQueue that had some AUs
ending with an EOS unit, but then followed later by some more AUs and eventually
another EOS unit? Would this even be possible, or is an AUQueue guaranteed to
not contain more than one EOS unit due to the way it is populated and drained?
Please add some comments at least around this.

[Tima Vaisburd, 2015/06/09 21:29:21]

If you consider this class separately, I do not see what prevents it. I think,
theoretically, yes.
The second and later EOS should not have any effect: after consuming first EOS
from the queue decoder should not call Advance() unless there is a Flush(). I
guess the best way would be blocking the input if EOS is pushed into the queue.

Just double checking: is more data after EOS the real option (i.e. AU, AU, EOS,
AU, AU, ... )?

[wolenetz, 2015/06/11 19:21:20]

Blocking any input after EOS (with some DCHECK or CHECK(!has_eos) on input), and
caller using Flush() to clear all including any EOS sgtm. Essentially, guarantee
that, if has_eos is true, then the last AU in the queue is that EOS AU.

More data after EOS could happen due to web app behavior: it's entirely allowed
for an MSE web app to append some data, then call MediaSource.endOfStream()
(without any optional error argument), the player to play to 'ended' state, then
the app to either seek or append more data. In the seek case, the AUQueue should
be flushed and repopulated by reading from the demuxer. In either case, once an
EOS has been read from the AUqueue, the caller can track the right behavior and
remove the EOS or flush the queue. 

[Tima Vaisburd, 2015/06/11 20:54:08]

I did the block. I'm not sure what to DCHECK though: as far as I understood,
the data after first EOS is a valid input.
It can be empty if we consumed everything.
The Flush() works as you describe as far as I can tell.

+    bool has_eos;
+
+    Info()
+        : front_unit(nullptr), configs(nullptr), length(0), has_eos(false) {}
+  };
+
+  AccessUnitQueue();
+  ~AccessUnitQueue();
+
+  // Appends the incoming data to the queue.
+  void PushBack(const DemuxerData& frames);
+
+  // Advances the front position to next unit. Logically the preceding units
+  // do not exist, but they can be physically removed later.
+  void Advance();
+
+  // Clears the queue.
+  void Flush();
+
+  // Looks back for the first key frame and if it exists, sets the current
+  // access unit to it and returns true. Otherwise returns false.

[wolenetz, 2015/06/08 21:37:09]

nit: Is the queue unmodified if the current "front position" is a keyframe (and
I assume true would be returned in this case, too). In other words, is the
look-back inclusive of the current front position? Please clarify in comments
here.

[Tima Vaisburd, 2015/06/09 21:29:21]

Inclusive. I changed the comment.

+  bool SkipToKeyFrame();

[wolenetz, 2015/06/08 21:37:09]

nit: s/SkipToKeyFrame()/RewindToLastKeyFrame()/ ?

[Tima Vaisburd, 2015/06/09 21:29:21]

Done.

+
+  // Returns the information about the queue.
+  Info GetInfo() const;
+
+  // For unit tests only.
+  size_t NumChunksForTesting() const { return chunks_.size(); }
+
+ private:
+  // Returns the amount of access units that has not been passed to decoder yet.

[wolenetz, 2015/06/08 21:37:09]

nit: This class doesn't pass to a decoder, so shouldn't leak its users' logic.

[Tima Vaisburd, 2015/06/09 21:29:21]

Done.

+  int GetUndecodedAccessUnitLength() const;
+
+  // The queue of data chunks.
+  typedef std::list<scoped_ptr<DemuxerData>> DataChunkQueue;

[wolenetz, 2015/06/08 21:37:09]

This doesn't look right to me. See "I want to use an STL container to hold
pointers.  Can I use smart pointers?" section at
https://www.chromium.org/developers/smart-pointer-guidelines.

[Tima Vaisburd, 2015/06/09 21:29:21]

Interesting. There is a lot of examples of using unique_ptr in STL containers,
including Google's own C++11 course. The link refers to "Effective STL" for the
rational. As far as I can get the rational is that copy ctor does not have copy
semantics and thus algorithms have undefined behavior. Again, only as far as I
understand, by not having copy ctor the unique_ptr will only allow algorithms
that do not expect the copy semantics. I thought scoped_ptr can be used as
unique_ptr replacement. 

[wolenetz, 2015/06/11 19:21:20]

I don't think scoped_ptr can be a unique_ptr replacement. STL container
internals don't use scoped_ptr.Pass(), TakesOwnership(), etc, when they move
things around in memory. I could be wrong, but I defer to the referenced
guidelines.

If ScopedVector is insufficient, then std::list<raw pointer> or
std::list<ScopedRefPtr..>. Use the former ONLY if there is guaranteed to be
exactly one user of the pointed-to-object at a time, and document it well. See
stl_utils for some tools if going that route. ScopedRefPtr is for when you're
not sure how many owners there might be at a time, and hopefully we don't need
that here (though we cannot guarantee exactly one caller of the GetInfo(), so at
least document that very well if going the raw ptr route.)

[Tima Vaisburd, 2015/06/11 20:54:08]

I went with raw pointer, the decoder seems to provide the guarantees you've
mentioned. I added more documentation in the header.

[Tima Vaisburd, 2015/06/11 21:32:04]

aside: Exactly this makes me believe that invalid usage of scoped_ptr in STL
containers or algorithms won't compile. This does not change the fact that I
happily went with raw pointer in this CL.

+  DataChunkQueue chunks_;
+
+  // The chunk that contains the current access unit.
+  DataChunkQueue::iterator current_chunk_;
+
+  // Index of the current access unit within the current chunk.
+  size_t index_in_chunk_;
+
+  // Indicates that a unit with End Of Stream flag has been appended.
+  bool has_eos_;
+
+  // The lock protects all fields together.
+  mutable base::Lock lock_;
+
+  DISALLOW_COPY_AND_ASSIGN(AccessUnitQueue);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_ANDROID_ACCESS_UNIT_QUEUE_H_