Access unit queue for MediaCodecPlayer

media/base/android/access_unit_queue.cc

Index: media/base/android/access_unit_queue.cc
diff --git a/media/base/android/access_unit_queue.cc b/media/base/android/access_unit_queue.cc
new file mode 100644
index 0000000000000000000000000000000000000000..236cd81a0d07ccfe4bdb57a217afefef1a26e6c4
--- /dev/null
+++ b/media/base/android/access_unit_queue.cc
@@ -0,0 +1,151 @@
+// 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.
+
+#include "media/base/android/access_unit_queue.h"
+
+#include "base/logging.h"
+#include "media/base/demuxer_stream.h"
+
+namespace media {
+
+AccessUnitQueue::AccessUnitQueue()
+    : index_in_chunk_(0),
+      has_eos_(false) {
+  current_chunk_ = chunks_.end();
+}
+
+AccessUnitQueue::~AccessUnitQueue() {}
+
+void AccessUnitQueue::PushBack(const DemuxerData& data) {
+  // Media thread
+  DCHECK(!data.access_units.empty());
+
+#if DCHECK_IS_ON()
+  // Verify the rule "one DemuxerConfigs for one AU with |kConfigChanged|".

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

Like EOS (and more simple vs the current verification code), kConfigChanged must
be the last unit in the chunk (if I read media_source_delegate.cc's
OnBufferReady() correctly).

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

Changed the comment and simplified the check.

+  size_t num_config_placeholders = 0;
+
+  for (size_t i = 0; i < data.access_units.size(); ++i) {
+    const AccessUnit& unit = data.access_units[i];
+    if (unit.status == DemuxerStream::kConfigChanged)
+      ++num_config_placeholders;
+
+    // EOS must be the last unit in the chunk
+    if (unit.is_end_of_stream) {
+      DCHECK(i == data.access_units.size() - 1);
+    }
+  }
+#endif
+
+  DCHECK(data.demuxer_configs.size() == num_config_placeholders);
+
+  // Also verify that there is no more than one |kConfigChanged| per chunk.
+  DCHECK(num_config_placeholders <= 1);

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

num_config_placeholders is undefined here in non-DCHECK'ed builds.

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

Removed since the checking code is simplified.

+
+  // Create the next chunk and copy data to it.
+  scoped_ptr<DemuxerData> chunk(new DemuxerData(data));
+
+  // EOS flag can only be in the last access unit.
+  bool has_eos = chunk->access_units.back().is_end_of_stream;
+
+  // Append this chunk to the queue.
+  base::AutoLock lock(lock_);
+
+  bool was_empty = (current_chunk_ == chunks_.end());
+
+  chunks_.push_back(chunk.Pass());
+
+  // Position the current chunk.
+  if (was_empty) {
+    current_chunk_ = --chunks_.end();
+    index_in_chunk_ = 0;
+  }
+
+  // We expect that the chunk containing EOS is the last chunk.
+  DCHECK(!has_eos_);
+  has_eos_ = has_eos;
+}
+
+void AccessUnitQueue::Advance() {
+  // Decoder thread
+  base::AutoLock lock(lock_);
+
+  if (current_chunk_ == chunks_.end())
+    return;
+
+  ++index_in_chunk_;
+  if ((*current_chunk_)->access_units.size() <= index_in_chunk_) {
+    index_in_chunk_ = 0;
+    ++current_chunk_;
+  }
+
+  // Remove all chunks before the current
+  DataChunkQueue::iterator first_to_keep = current_chunk_;
+  chunks_.erase(chunks_.begin(), first_to_keep);
+}
+
+void AccessUnitQueue::Flush() {
+  // Media thread
+  base::AutoLock lock(lock_);
+
+  chunks_.clear();
+
+  current_chunk_ = chunks_.end();
+  index_in_chunk_ = 0;
+  has_eos_ = false;
+}
+
+bool AccessUnitQueue::SkipToKeyFrame() {
+  // Media thread
+  base::AutoLock lock(lock_);
+
+  // Search backwards in the current chunk only.

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

This isn't clear from the .h comment. Also, why is there this restriction to
current chunk only?

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

Updated the comment in .h.

I searched in the current chunk only because currently we do not keep the old
chunk (i.e. the history is just up to 4 units of the current chunk).
With this patch I removed the assumption that there is one chunk only and
extended the search back. I also set the history size (in chunks) controllable.
I do not know, however, whether the increased complexity worth it. Please take a
look.

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

I tend to agree that increased complexity might not be worth it (see my first
comment). Some comment in Advance() about not keeping the advanced-from-chunk if
the advance moved it to the next chunk would be useful if dropping the history
piece. Sorry if my original comment churned you on this. "in the current chunk
only" triggered me to wonder why, and if the API were meant to have 0 chunk
history, then that comment makes more sense.
However, if trying to avoid BrowserSeeks (up to some max double-caching-with
chunkdemuxer threshold), then history might make sense.

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

That's no problem in general, and I went forward without asking in particular.
But some of your other comments leave an impression that it's actually ok to
keep.
Please clarify, until then I'll keep it.
Exactly, I thought we might want to play with the number.

+
+  if (current_chunk_ == chunks_.end())
+    return false;
+
+  for (int i = index_in_chunk_; i >= 0; --i) {

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

minor minor nit: On some platforms (like Windows), this assignment from size_t
to int causes truncation, compile warning, and build error. Just FYI since this
is Android, but please refrain from introducing this type of potential error in
case this object were ever to be used on some other platform :)
Also note that making "i" a "size_t" is also bad because it will then always be
>= 0 :/

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

I did not find an elegant solution, just put a cast to int.

+    if ((*current_chunk_)->access_units[i].is_key_frame) {
+      index_in_chunk_ = i;
+      return true;
+    }
+  }
+
+  return false;
+}
+
+AccessUnitQueue::Info AccessUnitQueue::GetInfo() const {
+  // Media thread, Decoder thread
+
+  Info info;
+  base::AutoLock lock(lock_);
+
+  info.length = GetUndecodedAccessUnitLength();
+  info.has_eos = has_eos_;
+  info.front_unit = nullptr;
+  info.configs = nullptr;
+
+  if (info.length > 0) {
+    DCHECK(current_chunk_ != chunks_.end());
+    DCHECK(index_in_chunk_ < (*current_chunk_)->access_units.size());
+    info.front_unit = &(*current_chunk_)->access_units[index_in_chunk_];

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

Who owns the DemuxerData scoped pointer at this point? See my comment in .h:
should we use a ScopedVector for chunks_ instead of std::list?

Also, since this is essentially a "Peek" operation, perhaps the DemuxerData
pointer should be refcounted since both ScopedVector and |info| reference it?

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

The container remains to be the owner. I wanted to return a non-owning pointer
while having container to own the object. I brought my consideration why I think
it's possible to use such container in h:

But if you say that this kind of interface is ugly, I'd humbly agree.
I wanted to avoid extra locks. Maybe I shouldn't have.
I used a list because it does not invalidate the iterator to the current chunk.
Will scoped_refptr in the container and Info eliminate your concerns?

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

See my other comments.

+
+    if (info.front_unit->status == DemuxerStream::kConfigChanged) {
+      DCHECK((*current_chunk_)->demuxer_configs.size() == 1);
+      info.configs = &(*current_chunk_)->demuxer_configs[0];

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

Likewise: ownership?

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

Same as above

+    }
+  }
+  return info;
+}
+
+int AccessUnitQueue::GetUndecodedAccessUnitLength() const {
+  int result = 0;
+  DataChunkQueue::const_iterator chunk;
+  for (chunk = current_chunk_; chunk != chunks_.end(); ++chunk)
+    result += (*chunk)->access_units.size();
+
+  result -= index_in_chunk_;
+  return result;
+}
+
+}  // namespace media