Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196133020:
Reducing the IPC latency for MSE video decoding (Closed)
|
Created:
6 years, 9 months ago by qinmin Modified:
6 years, 9 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReducing the IPC latency for MSE video decoding
For MSE, we decode the data in the browser process. When all the data is decoded, we started to request data from the render process.
So the decoder is idle during the IPC round trip + data copy time
This may introduce some issues as this delay may impact smooth video playbacks.
This change tries to solve the above problem:
1. Divide data into 2 chunks in the browser process
2. When decoding 1 chunk, check if the chunk is reaching its end.
If so, request data for the current chunk, and start decoding the next chunk.
3. When new data arrives, store them in the next chunk (since the current one might still be being decoded)
This change also:
1. reduced the buffered data size at the browser side
2. fixes crbug.com/306314 if there is pending data requests across release()/start()
3. fixed all the unit tests behaviors. The new approach should be well tested by all the unit tests we have.
BUG=306314
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258419
Patch Set 1 #
Total comments: 28
Patch Set 2 : addressing comments #
Total comments: 25
Patch Set 3 : addressing comments #
Total comments: 31
Patch Set 4 : nits #Patch Set 5 : rebase #Patch Set 6 : fix clang warning #Messages
Total messages: 23 (0 generated)
PTAL
Nice CL! Do you have any latency numbers or even empirical/anecdotal evidence of previous problem/improvement by this CL? Here's my first round of comments: https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:225: // When |input_eos_encountered_| is set, |access_unit| must be pointing to an s/access_unit/access_unit_index_/ Also, mention dependency on |current_demuxer_data_index_| (which should also be held constant when |input_eos_encoundered_| is set). Does this condition not hold true for ClearData(), which changes |current_demuxer_data_index_|, so there's a bug maybe? https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:280: access_unit, Is it possible for ClearData() to occur while the decoder thread still has reference |access_unit|? If so, does this introduce UAF race bug? Who owns the lifetime of each DemuxerData() in received_data_[]? https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:452: const AccessUnit& MediaDecoderJob::CurrentAccessUnit() const { nit: clarify with DCHECK any assumption of which thread this method is allowed to be called on. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:454: int index = IsDemuxerDataEmpty(true) ? 1- current_demuxer_data_index_ : nit: s/1-/1 -/ https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:454: int index = IsDemuxerDataEmpty(true) ? 1- current_demuxer_data_index_ : nit: extract "1 - current_demuxer_data_index_" to a(n inline) function like size_t inactive_demuxer_data_index_() ? https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:459: bool MediaDecoderJob::IsDemuxerDataEmpty(bool current_chunk) const { nit: ditto thread dcheck https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:465: void MediaDecoderJob::ClearData() { nit: ditto thread dcheck https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:466: current_demuxer_data_index_ = 0; ditto: If |input_eos_encountered_| is set, we should not change |current_demuxer_data_index_|. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:467: for (size_t i = 0; i < 2; ++i) { nit: Elevate this to a helper function, also use in constructor? https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:476: void MediaDecoderJob::RequestCurrentChunkIfEmpty() { nit: ditto thread dcheck https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.h:120: const base::TimeDelta& start_presentation_timestamp); nit: const & for TimeDelta is not current style. Underlying data is just an int64. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.h:144: // Check whether a chunk is empty. nit: Describe meaning of |current_chunk|. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_sou... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/196133020/diff/1/media/base/android/media_sou... media/base/android/media_source_player.cc:351: if (has_pending_audio_data_request_ && data.type == DemuxerStream::AUDIO) { Do I understand correctly that we drop on the floor the result of the pending read if the player has been Release()'ed and not yet restarted when the video or audio data arrives here? Is that the right thing to do, or should we hold onto the data to feed newly created decoder, or do we always seek to take care of this case on restart? https://codereview.chromium.org/196133020/diff/1/media/base/android/media_sou... media/base/android/media_source_player.cc:535: // If one of the decoder is not initialized, cancel this event as it will be Were we hitting the DCHECK's below? In other words, is it possible for PREFETCH_REQUEST_EVENT_PENDING to become true but the relevant decoder job(s) to not yet be initialized?
https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:225: // When |input_eos_encountered_| is set, |access_unit| must be pointing to an Done. Good point. ClearData() was called in 2 functions： flush() and Decode(). In flush(), it is immediately followed by a input_eos_encountered = false. In decode(), the access unit should be a config change. So we are not reaching EOS yet. Anyway, we should make the assumption consistent before and after calling ClearData(). Setting input_eos_encountered to false in ClearData(). On 2014/03/17 19:51:00, wolenetz wrote: > s/access_unit/access_unit_index_/ > > Also, mention dependency on |current_demuxer_data_index_| (which should also be > held constant when |input_eos_encoundered_| is set). > > Does this condition not hold true for ClearData(), which changes > |current_demuxer_data_index_|, so there's a bug maybe? https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:280: access_unit, No, ClearData() will be called only when the decoder thread is idle. ClearData() is called inside either Decode() or Flush() function. Flush() is called in ProcessPendingEvents() in MSP, Decode() is called when decoder is idle. So for both cases, the decoder thread should be idle. On 2014/03/17 19:51:00, wolenetz wrote: > Is it possible for ClearData() to occur while the decoder thread still has > reference |access_unit|? If so, does this introduce UAF race bug? > Who owns the lifetime of each DemuxerData() in received_data_[]? https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:452: const AccessUnit& MediaDecoderJob::CurrentAccessUnit() const { On 2014/03/17 19:51:00, wolenetz wrote: > nit: clarify with DCHECK any assumption of which thread this method is allowed > to be called on. Done. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:454: int index = IsDemuxerDataEmpty(true) ? 1- current_demuxer_data_index_ : On 2014/03/17 19:51:00, wolenetz wrote: > nit: s/1-/1 -/ Done. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:454: int index = IsDemuxerDataEmpty(true) ? 1- current_demuxer_data_index_ : On 2014/03/17 19:51:00, wolenetz wrote: > nit: s/1-/1 -/ Done. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:459: bool MediaDecoderJob::IsDemuxerDataEmpty(bool current_chunk) const { On 2014/03/17 19:51:00, wolenetz wrote: > nit: ditto thread dcheck Done. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:465: void MediaDecoderJob::ClearData() { On 2014/03/17 19:51:00, wolenetz wrote: > nit: ditto thread dcheck Done. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:466: current_demuxer_data_index_ = 0; changed the function to set |input_eos_encountered_| to false. On 2014/03/17 19:51:00, wolenetz wrote: > ditto: If |input_eos_encountered_| is set, we should not change > |current_demuxer_data_index_|. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:467: for (size_t i = 0; i < 2; ++i) { On 2014/03/17 19:51:00, wolenetz wrote: > nit: Elevate this to a helper function, also use in constructor? Done. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:476: void MediaDecoderJob::RequestCurrentChunkIfEmpty() { On 2014/03/17 19:51:00, wolenetz wrote: > nit: ditto thread dcheck Done. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.h:120: const base::TimeDelta& start_presentation_timestamp); On 2014/03/17 19:51:00, wolenetz wrote: > nit: const & for TimeDelta is not current style. Underlying data is just an > int64. Done. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.h:144: // Check whether a chunk is empty. On 2014/03/17 19:51:00, wolenetz wrote: > nit: Describe meaning of |current_chunk|. Done. https://codereview.chromium.org/196133020/diff/1/media/base/android/media_sou... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/196133020/diff/1/media/base/android/media_sou... media/base/android/media_source_player.cc:351: if (has_pending_audio_data_request_ && data.type == DemuxerStream::AUDIO) { Yes, the purpose of this is to drop the request that has been made by a released player. We cannot pass the data to a newly created decoder, because the data might be requested for an inactive chunk, which is not the data the previous decoder is currently decoding. Additionally, the data may not be an iframe and we cannot feed it to a newly created decoder. On 2014/03/17 19:51:00, wolenetz wrote: > Do I understand correctly that we drop on the floor the result of the pending > read if the player has been Release()'ed and not yet restarted when the video or > audio data arrives here? Is that the right thing to do, or should we hold onto > the data to feed newly created decoder, or do we always seek to take care of > this case on restart? https://codereview.chromium.org/196133020/diff/1/media/base/android/media_sou... media/base/android/media_source_player.cc:535: // If one of the decoder is not initialized, cancel this event as it will be This is possible with the new implementation. Considering the case that we pass an empty surface to the video decoder: In that case, the old decoder will be destroyed while requesting some data. Since the new surface is empty, we will not create the new video decoder. In our previous implementation, we will early return when processing the SURFACE_CHANGE event. Now after sometime the data requested by the old decoder arrives, and we start to process pending events. Since SURFACE_CHANGE event has been processed, we will hit the PREFETCH_REQUEST pending event and find that the video decoder is actually null. When decoder is null, there is no point to handle the prefetch request, that's why we can clear the pending events here. On 2014/03/17 19:51:00, wolenetz wrote: > Were we hitting the DCHECK's below? In other words, is it possible for > PREFETCH_REQUEST_EVENT_PENDING to become true but the relevant decoder job(s) to > not yet be initialized?
Thanks for addressing comments. Here's my next round (mostly nits): https://codereview.chromium.org/196133020/diff/20001/content/renderer/media/a... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/196133020/diff/20001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.cc:35: const size_t kAccessUnitSizeForMediaSource = 4; nit (mostly just curious): why not 16? (or 8 since we're doubling the number of "in-flight" chunks)? Smaller numbers here may reduce some of the browser seeking failing to seek to target due to GC in the underlying chunkdemuxer. Is there a drop in latency and rebuffering/janky playback delays with this change to two chunks but overall half the previous access units in flight? https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:59: // If there is a pending callback, need to request the data again to get nit: insert line before comment? https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:224: // We'll reuse this unit to flush the decoder until we hit outpu EOS. nit: s/outpu/output/ https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:457: bool MediaDecoderJob::IsDemuxerDataEmpty(bool current_chunk) const { nit: I'm not too sure this is the best name. Maybe rename to "NoAccessUnitsRemaining[InChunk or DemuxerData]" or invert the logic+implementation and rename to "AccessUnitAvailableInDemuxerData"? I have no strong feeling here. https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_decoder_job.h:83: return is_requesting_demuxer_data_; nit: Does all of this fit on one line? https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player.cc:367: nit: remove extra lines (both of these) https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player.cc:439: base::TimeDelta seek_time = (actual_browser_seek_time == kNoTimestamp()) ? See also comment in unit tests. Why are we now allowing kNoTimestamp() for browser seek actual seek time? https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:952: player_.OnDemuxerSeekDone(kNoTimestamp()); Previously, browser seeks were required to indicate the timestamp that they seeked to. This change (and as it is used here) allows kNoTimestamp(). Why? https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1041: while(GetMediaDecoderJob(true)->is_decoding()) nit: s/while(/while (/ https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1043: // The decoder job should finish and waiting for data. nit: s/waiting/wait/ or s/waiting/be waiting/ https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1084: player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(i)); nit: Why drop the EXPECT_TRUE(Get...->is_decoding()) ? https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1086: WaitForAudioDecodeDone(); nit: odd indentation
On 2014/03/18 21:14:46, wolenetz wrote: > Thanks for addressing comments. Here's my next round (mostly nits): > > https://codereview.chromium.org/196133020/diff/20001/content/renderer/media/a... > File content/renderer/media/android/media_source_delegate.cc (right): > > https://codereview.chromium.org/196133020/diff/20001/content/renderer/media/a... > content/renderer/media/android/media_source_delegate.cc:35: const size_t > kAccessUnitSizeForMediaSource = 4; > nit (mostly just curious): why not 16? (or 8 since we're doubling the number of > "in-flight" chunks)? Smaller numbers here may reduce some of the browser seeking > failing to seek to target due to GC in the underlying chunkdemuxer. Is there a > drop in latency and rebuffering/janky playback delays with this change to two > chunks but overall half the previous access units in flight? > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > File media/base/android/media_decoder_job.cc (right): > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > media/base/android/media_decoder_job.cc:59: // If there is a pending callback, > need to request the data again to get > nit: insert line before comment? > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > media/base/android/media_decoder_job.cc:224: // We'll reuse this unit to flush > the decoder until we hit outpu EOS. > nit: s/outpu/output/ > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > media/base/android/media_decoder_job.cc:457: bool > MediaDecoderJob::IsDemuxerDataEmpty(bool current_chunk) const { > nit: I'm not too sure this is the best name. Maybe rename to > "NoAccessUnitsRemaining[InChunk or DemuxerData]" or invert the > logic+implementation and rename to "AccessUnitAvailableInDemuxerData"? I have no > strong feeling here. > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > File media/base/android/media_decoder_job.h (right): > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > media/base/android/media_decoder_job.h:83: return is_requesting_demuxer_data_; > nit: Does all of this fit on one line? > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > File media/base/android/media_source_player.cc (right): > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > media/base/android/media_source_player.cc:367: > nit: remove extra lines (both of these) > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > media/base/android/media_source_player.cc:439: base::TimeDelta seek_time = > (actual_browser_seek_time == kNoTimestamp()) ? > See also comment in unit tests. Why are we now allowing kNoTimestamp() for > browser seek actual seek time? > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > File media/base/android/media_source_player_unittest.cc (right): > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:952: > player_.OnDemuxerSeekDone(kNoTimestamp()); > Previously, browser seeks were required to indicate the timestamp that they > seeked to. This change (and as it is used here) allows kNoTimestamp(). Why? > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:1041: > while(GetMediaDecoderJob(true)->is_decoding()) > nit: s/while(/while (/ > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:1043: // The decoder job > should finish and waiting for data. > nit: s/waiting/wait/ or s/waiting/be waiting/ > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:1084: > player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(i)); > nit: Why drop the EXPECT_TRUE(Get...->is_decoding()) ? > > https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:1086: > WaitForAudioDecodeDone(); > nit: odd indentation Also a couple nits in the CL description: s/beging/being/ and s/requesting/request/
I did some experiment with the patch using http://dash-mse-test.appspot.com/dash-player.html?url=http://yt-dash-mse-test.... Without the patch, the average time that decoder is idle （no data coming) for more than 1ms is: 382 (idle for 1ms+） 54 (idle for 5ms+） 12 (idle for 10ms+） With this patch: 63 (idle for 1ms+） 9 (idle for 5ms+） 6 (idle for 10ms+） https://codereview.chromium.org/196133020/diff/20001/content/renderer/media/a... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/196133020/diff/20001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.cc:35: const size_t kAccessUnitSizeForMediaSource = 4; This is mainly to reduce the memory usage. We currently store 16 AUs in browser process, and each time also requesting 16. By reducing the AU size to 4, we can only store 8 AUs in the browser process. I did not observe any rebuffering/janky differences between 4 and 8 On 2014/03/18 21:14:47, wolenetz wrote: > nit (mostly just curious): why not 16? (or 8 since we're doubling the number of > "in-flight" chunks)? Smaller numbers here may reduce some of the browser seeking > failing to seek to target due to GC in the underlying chunkdemuxer. Is there a > drop in latency and rebuffering/janky playback delays with this change to two > chunks but overall half the previous access units in flight? https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:59: // If there is a pending callback, need to request the data again to get On 2014/03/18 21:14:47, wolenetz wrote: > nit: insert line before comment? Done. https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:224: // We'll reuse this unit to flush the decoder until we hit outpu EOS. On 2014/03/18 21:14:47, wolenetz wrote: > nit: s/outpu/output/ Done. https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:457: bool MediaDecoderJob::IsDemuxerDataEmpty(bool current_chunk) const { renamed to NoAccessUnitsRemainingInChunk. On 2014/03/18 21:14:47, wolenetz wrote: > nit: I'm not too sure this is the best name. Maybe rename to > "NoAccessUnitsRemaining[InChunk or DemuxerData]" or invert the > logic+implementation and rename to "AccessUnitAvailableInDemuxerData"? I have no > strong feeling here. https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_decoder_job.h:83: return is_requesting_demuxer_data_; just 1 space short :( On 2014/03/18 21:14:47, wolenetz wrote: > nit: Does all of this fit on one line? https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player.cc:367: On 2014/03/18 21:14:47, wolenetz wrote: > nit: remove extra lines (both of these) Done. https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player.cc:439: base::TimeDelta seek_time = (actual_browser_seek_time == kNoTimestamp()) ? I saw the kNoTimestamp() in unit tests and it makes me unsure about what the chunk demuxer may return. Removed this if we can guarantee there will be no kNoTimestamp from the demuxer. On 2014/03/18 21:14:47, wolenetz wrote: > See also comment in unit tests. Why are we now allowing kNoTimestamp() for > browser seek actual seek time? https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:952: player_.OnDemuxerSeekDone(kNoTimestamp()); I saw kNoTimestamp was used many places in this file. Correcting all those places now. On 2014/03/18 21:14:47, wolenetz wrote: > Previously, browser seeks were required to indicate the timestamp that they > seeked to. This change (and as it is used here) allows kNoTimestamp(). Why? https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1041: while(GetMediaDecoderJob(true)->is_decoding()) On 2014/03/18 21:14:47, wolenetz wrote: > nit: s/while(/while (/ Done. https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1043: // The decoder job should finish and waiting for data. On 2014/03/18 21:14:47, wolenetz wrote: > nit: s/waiting/wait/ or s/waiting/be waiting/ Done. https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1084: player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(i)); added back. On 2014/03/18 21:14:47, wolenetz wrote: > nit: Why drop the EXPECT_TRUE(Get...->is_decoding()) ? https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1086: WaitForAudioDecodeDone(); On 2014/03/18 21:14:47, wolenetz wrote: > nit: odd indentation Done.
Thanks for updating. This round, all my comments are nits. lgtm % nits https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/196133020/diff/20001/media/base/android/media... media/base/android/media_decoder_job.h:83: return is_requesting_demuxer_data_; On 2014/03/19 02:45:22, qinmin wrote: > just 1 space short :( > > On 2014/03/18 21:14:47, wolenetz wrote: > > nit: Does all of this fit on one line? > Sorry, my miscalculation (I missed the indentation spaces.) https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_decoder_job.cc:459: bool MediaDecoderJob::NoAccessUnitsRemainingInChunk(bool current_chunk) const { nit: s/current_chunk/is_active_chunk/ to match declaration. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_decoder_job.cc:461: size_t index = current_chunk ? current_demuxer_data_index_ : nit: ditto https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player.cc:448: nit: Due to confusion in |actual_browser_seek_time| usage (see my current round of unit test comments), perhaps add a DCHECK that enforces actual_browser_seek_time == kNoTimestamp() if and only if NOT |doing_browser_seek|? See also https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:446: player_.OnDemuxerSeekDone(base::TimeDelta()); I think there is some confusion around OnDemuxerSeekDone() expected usage. If (and only if) the renderer is signaling completion of a browser seek, it *must* pass the actual browser seek target time, since it may differ from the requested browser seek target time due to renderer-side chunkdemuxer GC. However, if the renderer is signaling completion of a non-browser-seek (just a regular seek), then the parameter should be kNoTimestamp() here. See also https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... This test helper method is used for only regular seeks, so it should just pass kNoTimestamp() here. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:673: player_.OnDemuxerSeekDone(base::TimeDelta()); ditto: pass kNoTimestamp() here. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:726: player_.OnDemuxerSeekDone(base::TimeDelta()); ditto: pass kNoTimestamp() here. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:856: player_.OnDemuxerSeekDone(base::TimeDelta()); ditto: pass kNoTimestamp() here (no browser seek should have started, and is indeed confirmed to not have occurred below, since a regular seek was already being processed when the surface was changed). https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:952: player_.OnDemuxerSeekDone(base::TimeDelta()); This is correct: it signals a browser seek completion with the actual seeked-to-time of 0. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:981: player_.OnDemuxerSeekDone(base::TimeDelta()); ditto: pass kNoTimestamp() here. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1008: player_.OnDemuxerSeekDone(base::TimeDelta()); ditto: pass kNoTimestamp() here. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1395: player_.OnDemuxerSeekDone(base::TimeDelta()); ditto: pass kNoTimestamp() here. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1738: player_.OnDemuxerSeekDone(base::TimeDelta()); ditto: pass kNoTimestamp() here. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1849: player_.OnDemuxerSeekDone(base::TimeDelta()); ditto: pass kNoTimestamp() here. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1882: player_.OnDemuxerSeekDone(base::TimeDelta()); ditto: pass kNoTimestamp() here. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1904: player_.OnDemuxerSeekDone(base::TimeDelta()); ditto: pass kNoTimestamp() here. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1935: player_.OnDemuxerSeekDone(base::TimeDelta()); ditto: pass kNoTimestamp() here.
https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_decoder_job.cc:459: bool MediaDecoderJob::NoAccessUnitsRemainingInChunk(bool current_chunk) const { On 2014/03/19 17:26:05, wolenetz wrote: > nit: s/current_chunk/is_active_chunk/ to match declaration. Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_decoder_job.cc:461: size_t index = current_chunk ? current_demuxer_data_index_ : On 2014/03/19 17:26:05, wolenetz wrote: > nit: ditto Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player.cc:448: On 2014/03/19 17:26:05, wolenetz wrote: > nit: Due to confusion in |actual_browser_seek_time| usage (see my current round > of unit test comments), perhaps add a DCHECK that enforces > actual_browser_seek_time == kNoTimestamp() if and only if NOT > |doing_browser_seek|? > > See also > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:446: player_.OnDemuxerSeekDone(base::TimeDelta()); On 2014/03/19 17:26:05, wolenetz wrote: > I think there is some confusion around OnDemuxerSeekDone() expected usage. > If (and only if) the renderer is signaling completion of a browser seek, it > *must* pass the actual browser seek target time, since it may differ from the > requested browser seek target time due to renderer-side chunkdemuxer GC. > > However, if the renderer is signaling completion of a non-browser-seek (just a > regular seek), then the parameter should be kNoTimestamp() here. > > See also > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... > > This test helper method is used for only regular seeks, so it should just pass > kNoTimestamp() here. Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:673: player_.OnDemuxerSeekDone(base::TimeDelta()); On 2014/03/19 17:26:05, wolenetz wrote: > ditto: pass kNoTimestamp() here. Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:726: player_.OnDemuxerSeekDone(base::TimeDelta()); On 2014/03/19 17:26:05, wolenetz wrote: > ditto: pass kNoTimestamp() here. Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:856: player_.OnDemuxerSeekDone(base::TimeDelta()); On 2014/03/19 17:26:05, wolenetz wrote: > ditto: pass kNoTimestamp() here (no browser seek should have started, and is > indeed confirmed to not have occurred below, since a regular seek was already > being processed when the surface was changed). Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:981: player_.OnDemuxerSeekDone(base::TimeDelta()); On 2014/03/19 17:26:05, wolenetz wrote: > ditto: pass kNoTimestamp() here. Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1008: player_.OnDemuxerSeekDone(base::TimeDelta()); On 2014/03/19 17:26:05, wolenetz wrote: > ditto: pass kNoTimestamp() here. Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1395: player_.OnDemuxerSeekDone(base::TimeDelta()); On 2014/03/19 17:26:05, wolenetz wrote: > ditto: pass kNoTimestamp() here. Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1738: player_.OnDemuxerSeekDone(base::TimeDelta()); On 2014/03/19 17:26:05, wolenetz wrote: > ditto: pass kNoTimestamp() here. Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1849: player_.OnDemuxerSeekDone(base::TimeDelta()); On 2014/03/19 17:26:05, wolenetz wrote: > ditto: pass kNoTimestamp() here. Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1882: player_.OnDemuxerSeekDone(base::TimeDelta()); On 2014/03/19 17:26:05, wolenetz wrote: > ditto: pass kNoTimestamp() here. Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1904: player_.OnDemuxerSeekDone(base::TimeDelta()); On 2014/03/19 17:26:05, wolenetz wrote: > ditto: pass kNoTimestamp() here. Done. https://codereview.chromium.org/196133020/diff/40001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1935: player_.OnDemuxerSeekDone(base::TimeDelta()); On 2014/03/19 17:26:05, wolenetz wrote: > ditto: pass kNoTimestamp() here. Done.
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/196133020/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for media/base/android/media_decoder_job.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file media/base/android/media_decoder_job.cc
Hunk #1 FAILED at 33.
Hunk #2 succeeded at 43 (offset -1 lines).
Hunk #3 succeeded at 93 (offset -1 lines).
Hunk #4 succeeded at 103 (offset -1 lines).
Hunk #5 succeeded at 131 (offset -1 lines).
Hunk #6 FAILED at 216.
Hunk #7 succeeded at 231 (offset -2 lines).
Hunk #8 succeeded at 274 (offset -2 lines).
Hunk #9 succeeded at 283 (offset -2 lines).
Hunk #10 succeeded at 409 (offset 1 line).
Hunk #11 succeeded at 432 (offset 1 line).
Hunk #12 succeeded at 449 (offset 1 line).
2 out of 12 hunks FAILED -- saving rejects to file
media/base/android/media_decoder_job.cc.rej
Patch: media/base/android/media_decoder_job.cc
Index: media/base/android/media_decoder_job.cc
diff --git a/media/base/android/media_decoder_job.cc
b/media/base/android/media_decoder_job.cc
index
535b8d114e0a76194d3df307b0534bc3b50ab65f..989407df4b2c799a2de9cc7658adfdd10803b7ff
100644
--- a/media/base/android/media_decoder_job.cc
+++ b/media/base/android/media_decoder_job.cc
@@ -33,10 +33,13 @@ MediaDecoderJob::MediaDecoderJob(
prerolling_(true),
weak_this_(this),
request_data_cb_(request_data_cb),
- access_unit_index_(0),
+ current_demuxer_data_index_(0),
input_buf_index_(-1),
stop_decode_pending_(false),
- destroy_pending_(false) {
+ destroy_pending_(false),
+ is_requesting_demuxer_data_(false),
+ is_incoming_data_invalid_(false) {
+ InitializeReceivedData();
}
MediaDecoderJob::~MediaDecoderJob() {}
@@ -44,23 +47,38 @@ MediaDecoderJob::~MediaDecoderJob() {}
void MediaDecoderJob::OnDataReceived(const DemuxerData& data) {
DVLOG(1) << __FUNCTION__ << ": " << data.access_units.size() << " units";
DCHECK(ui_task_runner_->BelongsToCurrentThread());
- DCHECK(!on_data_received_cb_.is_null());
+ DCHECK(NoAccessUnitsRemainingInChunk(false));
TRACE_EVENT_ASYNC_END2(
"media", "MediaDecoderJob::RequestData", this,
"Data type", data.type == media::DemuxerStream::AUDIO ? "AUDIO" :
"VIDEO",
"Units read", data.access_units.size());
- base::Closure done_cb = base::ResetAndReturn(&on_data_received_cb_);
+ if (is_incoming_data_invalid_) {
+ is_incoming_data_invalid_ = false;
+
+ // If there is a pending callback, need to request the data again to get
+ // valid data.
+ if (!on_data_received_cb_.is_null())
+ request_data_cb_.Run();
+ else
+ is_requesting_demuxer_data_ = false;
+ return;
+ }
+ size_t next_demuxer_data_index = inactive_demuxer_data_index();
+ received_data_[next_demuxer_data_index] = data;
+ access_unit_index_[next_demuxer_data_index] = 0;
+ is_requesting_demuxer_data_ = false;
+
+ base::Closure done_cb = base::ResetAndReturn(&on_data_received_cb_);
if (stop_decode_pending_) {
OnDecodeCompleted(MEDIA_CODEC_STOPPED, kNoTimestamp(), 0);
return;
}
- access_unit_index_ = 0;
- received_data_ = data;
- done_cb.Run();
+ if (!done_cb.is_null())
+ done_cb.Run();
}
void MediaDecoderJob::Prefetch(const base::Closure& prefetch_cb) {
@@ -79,8 +97,8 @@ void MediaDecoderJob::Prefetch(const base::Closure&
prefetch_cb) {
}
bool MediaDecoderJob::Decode(
- const base::TimeTicks& start_time_ticks,
- const base::TimeDelta& start_presentation_timestamp,
+ 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());
@@ -89,23 +107,20 @@ bool MediaDecoderJob::Decode(
decode_cb_ = callback;
if (!HasData()) {
- RequestData(base::Bind(&MediaDecoderJob::DecodeNextAccessUnit,
+ RequestData(base::Bind(&MediaDecoderJob::DecodeCurrentAccessUnit,
base::Unretained(this),
start_time_ticks,
start_presentation_timestamp));
return true;
}
- if (DemuxerStream::kConfigChanged ==
- received_data_.access_units[access_unit_index_].status) {
+ if (DemuxerStream::kConfigChanged == CurrentAccessUnit().status) {
// Clear received data because we need to handle a config change.
decode_cb_.Reset();
- received_data_ = DemuxerData();
- access_unit_index_ = 0;
return false;
}
- DecodeNextAccessUnit(start_time_ticks, start_presentation_timestamp);
+ DecodeCurrentAccessUnit(start_time_ticks, start_presentation_timestamp);
return true;
}
@@ -120,14 +135,10 @@ void MediaDecoderJob::Flush() {
// Do nothing, flush when the next Decode() happens.
needs_flush_ = true;
- received_data_ = DemuxerData();
- input_eos_encountered_ = false;
- access_unit_index_ = 0;
- on_data_received_cb_.Reset();
+ ClearData();
}
-void MediaDecoderJob::BeginPrerolling(
- const base::TimeDelta& preroll_timestamp) {
+void MediaDecoderJob::BeginPrerolling(base::TimeDelta preroll_timestamp) {
DVLOG(1) << __FUNCTION__ << "(" << preroll_timestamp.InSecondsF() << ")";
DCHECK(ui_task_runner_->BelongsToCurrentThread());
DCHECK(!is_decoding());
@@ -208,16 +219,12 @@ MediaCodecStatus MediaDecoderJob::QueueInputBuffer(const
AccessUnit& unit) {
bool MediaDecoderJob::HasData() const {
DCHECK(ui_task_runner_->BelongsToCurrentThread());
- // When |input_eos_encountered_| is set, |access_units| must not be empty and
- // |access_unit_index_| must be pointing to an EOS unit. We'll reuse this
- // unit to flush the decoder until we hit output EOS.
- DCHECK(!input_eos_encountered_ ||
- (received_data_.access_units.size() > 0 &&
- access_unit_index_ < received_data_.access_units.size()))
- << " (access_units.size(): " << received_data_.access_units.size()
- << ", access_unit_index_: " << access_unit_index_ << ")";
- return access_unit_index_ < received_data_.access_units.size() ||
- input_eos_encountered_;
+ // When |input_eos_encountered_| is set, |access_unit_index_| and
+ // |current_demuxer_data_index_| must be pointing to an EOS unit.
+ // We'll reuse this unit to flush the decoder until we hit output EOS.
+ DCHECK(!input_eos_encountered_ || !NoAccessUnitsRemainingInChunk(true));
+ return !NoAccessUnitsRemainingInChunk(true) ||
+ !NoAccessUnitsRemainingInChunk(false);
}
void MediaDecoderJob::RequestData(const base::Closure& done_cb) {
@@ -225,26 +232,38 @@ void MediaDecoderJob::RequestData(const base::Closure&
done_cb) {
DCHECK(ui_task_runner_->BelongsToCurrentThread());
DCHECK(on_data_received_cb_.is_null());
DCHECK(!input_eos_encountered_);
+ DCHECK(NoAccessUnitsRemainingInChunk(false));
TRACE_EVENT_ASYNC_BEGIN0("media", "MediaDecoderJob::RequestData", this);
- received_data_ = DemuxerData();
- access_unit_index_ = 0;
on_data_received_cb_ = done_cb;
+ // If we are already expecting new data, just set the callback and do
+ // nothing.
+ if (is_requesting_demuxer_data_)
+ return;
+
+ // The new incoming data will be stored as the next demuxer data chunk, since
+ // the decoder might still be decoding the current one.
+ size_t next_demuxer_data_index = inactive_demuxer_data_index();
+ received_data_[next_demuxer_data_index] = DemuxerData();
+ access_unit_index_[next_demuxer_data_index] = 0;
+ is_requesting_demuxer_data_ = true;
+
request_data_cb_.Run();
}
-void MediaDecoderJob::DecodeNextAccessUnit(
- const base::TimeTicks& start_time_ticks,
- const base::TimeDelta& start_presentation_timestamp) {
+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
// the input buffer again so that it can request config data.
- if (received_data_.access_units[access_unit_index_].status ==
- DemuxerStream::kConfigChanged) {
+ if (access_unit.status == DemuxerStream::kConfigChanged) {
ui_task_runner_->PostTask(FROM_HERE,
base::Bind(&MediaDecoderJob::OnDecodeCompleted,
base::Unretained(this),
@@ -256,7 +275,7 @@ void MediaDecoderJob::DecodeNextAccessUnit(
decoder_task_runner_->PostTask(FROM_HERE, base::Bind(
&MediaDecoderJob::DecodeInternal, base::Unretained(this),
- received_data_.access_units[access_unit_index_],
+ access_unit,
start_time_ticks, start_presentation_timestamp, needs_flush_,
media::BindToCurrentLoop(base::Bind(
&MediaDecoderJob::OnDecodeCompleted, base::Unretained(this)))));
@@ -265,8 +284,8 @@ void MediaDecoderJob::DecodeNextAccessUnit(
void MediaDecoderJob::DecodeInternal(
const AccessUnit& unit,
- const base::TimeTicks& start_time_ticks,
- const base::TimeDelta& start_presentation_timestamp,
+ base::TimeTicks start_time_ticks,
+ base::TimeDelta start_presentation_timestamp,
bool needs_flush,
const MediaDecoderJob::DecoderCallback& callback) {
DVLOG(1) << __FUNCTION__;
@@ -388,7 +407,7 @@ void MediaDecoderJob::DecodeInternal(
}
void MediaDecoderJob::OnDecodeCompleted(
- MediaCodecStatus status, const base::TimeDelta& presentation_timestamp,
+ MediaCodecStatus status, base::TimeDelta presentation_timestamp,
size_t audio_output_bytes) {
DCHECK(ui_task_runner_->BelongsToCurrentThread());
@@ -411,7 +430,7 @@ void MediaDecoderJob::OnDecodeCompleted(
case MEDIA_CODEC_OUTPUT_FORMAT_CHANGED:
case MEDIA_CODEC_OUTPUT_END_OF_STREAM:
if (!input_eos_encountered_)
- access_unit_index_++;
+ access_unit_index_[current_demuxer_data_index_]++;
break;
case MED…
(message too large)
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/196133020/100001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/196133020/100001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/196133020/120001
Message was sent while issue was closed.
Change committed as 258419 |
