Remove the IPC to request DemuxerConfigs when config changes

media/base/android/media_decoder_job.cc

 // Copyright 2013 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/media_decoder_job.h"
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/debug/trace_event.h"
 #include "base/message_loop/message_loop_proxy.h"
@@ skipping 84 matching lines @@
   if (HasData()) {
     DVLOG(1) << __FUNCTION__ << " : using previously received data";
     ui_task_runner_->PostTask(FROM_HERE, prefetch_cb);
     return;
   }
 
   DVLOG(1) << __FUNCTION__ << " : requesting data";
   RequestData(prefetch_cb);
 }
 
 bool MediaDecoderJob::Decode(

[wolenetz, 2014/05/02 22:25:30]

It seems to me more understandable to just return a pointer to the configs
(instead of true) or NULL (instead of false), thereby eliminating need for
caller of Decode to poke us for the configs for these same conditions. We could
possibly pass a scoped_ptr back containing a copy of the DemuxerConfigs (or
NULL) and thereby retain the ClearData() (see my next comment).

[qinmin, 2014/05/05 20:52:19]

Done.

[wolenetz, 2014/05/05 22:06:55]

Oops - I swapped my false / true mapping in my original CR comment. Thanks for
catching!

     base::TimeTicks start_time_ticks,
     base::TimeDelta start_presentation_timestamp,
     const DecoderCallback& callback) {
   DCHECK(decode_cb_.is_null());
   DCHECK(on_data_received_cb_.is_null());
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
 
   decode_cb_ = callback;
 
   if (!HasData()) {
     RequestData(base::Bind(&MediaDecoderJob::DecodeCurrentAccessUnit,
                            base::Unretained(this),
                            start_time_ticks,
                            start_presentation_timestamp));
     return true;
   }
 
   if (DemuxerStream::kConfigChanged == CurrentAccessUnit().status) {
     // Clear received data because we need to handle a config change.
     decode_cb_.Reset();

[wolenetz, 2014/05/02 22:25:30]

I'm confused: previously we needed to ClearData(). Now, when does this occur
once we've reached this code path?

[qinmin, 2014/05/05 20:52:19]

If there is a config change, MediaDecoderJob will be recreated， thus there is no
need to clear the data here. 
Additionally, the data is still needed after this call because MSP needs it to
get the DemuxerConfig. And with my coming refactoring patch, we need to keep the
data for the inactive chunk after receiving a config change, so we don't need to
request it again.

On 2014/05/02 22:25:30, wolenetz wrote:

[wolenetz, 2014/05/05 22:06:55]

sgtm thanks

-    ClearData();
     return false;
   }
 
   DecodeCurrentAccessUnit(start_time_ticks, start_presentation_timestamp);
   return true;
 }
 
 void MediaDecoderJob::StopDecode() {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   DCHECK(is_decoding());
@@ skipping 10 matching lines @@
 
 void MediaDecoderJob::BeginPrerolling(base::TimeDelta preroll_timestamp) {
   DVLOG(1) << __FUNCTION__ << "(" << preroll_timestamp.InSecondsF() << ")";
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   DCHECK(!is_decoding());
 
   preroll_timestamp_ = preroll_timestamp;
   prerolling_ = true;
 }
 
+DemuxerConfigs* MediaDecoderJob::GetDemuxerConfigs() {
+  DVLOG(1) << __FUNCTION__;
+  DCHECK(ui_task_runner_->BelongsToCurrentThread());
+  DCHECK(!is_decoding());
+
+  int index = NoAccessUnitsRemainingInChunk(true) ?

[wolenetz, 2014/05/02 22:25:30]

nit: seems like some code duplication of CurrentAccessUnit() here. de-dupe with
some "current received data chunk" helper?

[qinmin, 2014/05/05 20:52:19]

Done.

+      inactive_demuxer_data_index() : current_demuxer_data_index_;
+  if (received_data_[index].demuxer_configs.size() > 0)
+    return &(received_data_[index].demuxer_configs[0]);
+  return NULL;
+}
+
 void MediaDecoderJob::Release() {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   DVLOG(1) << __FUNCTION__;
 
   // If the decoder job is not waiting for data, and is still decoding, we
   // cannot delete the job immediately.
   destroy_pending_ = on_data_received_cb_.is_null() && is_decoding();
 
   request_data_cb_.Reset();
   on_data_received_cb_.Reset();
@@ skipping 94 matching lines @@
 }
 
 void MediaDecoderJob::DecodeCurrentAccessUnit(
     base::TimeTicks start_time_ticks,
     base::TimeDelta start_presentation_timestamp) {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   DCHECK(!decode_cb_.is_null());
 
   RequestCurrentChunkIfEmpty();
   const AccessUnit& access_unit = CurrentAccessUnit();
   // If the first access unit is a config change, request the player to dequeue

[wolenetz, 2014/05/02 22:25:30]

nit: Now, if the first AU is a config change, shouldn't the configs already be
available in the "current received data chunk"? It seems to me (possibly not for
this CL) that this is a bit of a circuitous route. wdyt?

[qinmin, 2014/05/05 20:52:19]

When MSP calls decode(), if there is no data available, MDJ will call
requestData(). And the returned AU might be a config change. 
In this case, MSP already finishes calling decode(), and is expecting
OnDecodeCompleted() to be called. That's why we want MSP to call decode on the
same AU again.

On 2014/05/02 22:25:30, wolenetz wrote:

[wolenetz, 2014/05/05 22:06:55]

sgtm thanks

   // the input buffer again so that it can request config data.
   if (access_unit.status == DemuxerStream::kConfigChanged) {
     ui_task_runner_->PostTask(FROM_HERE,
                               base::Bind(&MediaDecoderJob::OnDecodeCompleted,
                                          base::Unretained(this),
                                          MEDIA_CODEC_DEQUEUE_INPUT_AGAIN_LATER,
                                          kNoTimestamp(), kNoTimestamp()));
     return;
   }
 
@@ skipping 207 matching lines @@
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   DCHECK(HasData());
   if (!NoAccessUnitsRemainingInChunk(true))
     return;
 
   // Requests new data if the the last access unit of the next chunk is not EOS.
   current_demuxer_data_index_ = inactive_demuxer_data_index();
   const AccessUnit last_access_unit =
       received_data_[current_demuxer_data_index_].access_units.back();
   if (!last_access_unit.end_of_stream &&
       last_access_unit.status != DemuxerStream::kConfigChanged &&

[wolenetz, 2014/05/02 22:25:30]

If it is kConfigChanged, and ClearData() never occurred, does this job become
unable to process any further data?

[qinmin, 2014/05/05 20:52:19]

if it is kConfigChanged, MDJ will get recreated and no further data needs to be
processed.

On 2014/05/02 22:25:30, wolenetz wrote:

[wolenetz, 2014/05/05 22:06:55]

sgtm thanks

       last_access_unit.status != DemuxerStream::kAborted) {
     RequestData(base::Closure());
   }
 }
 
 void MediaDecoderJob::InitializeReceivedData() {
   for (size_t i = 0; i < 2; ++i) {
     received_data_[i] = DemuxerData();
     access_unit_index_[i] = 0;
   }
 }
 
 }  // namespace media