Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1176993005:
Audio and video decoders for MediaCodecPlayer (Closed)
|
Created:
5 years, 6 months ago by Tima Vaisburd Modified:
5 years, 6 months ago CC:
chromium-reviews, posciak+watch_chromium.org, avayvod+watch_chromium.org, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mlamouri+watch-media_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRevert of Revert of "Audio and video decoders for MediaCodecPlayer"
The original CL has been reverted in
https://codereview.chromium.org/1194423008/, this is the original with the fix.
Original CL:
Audio and video decoders for MediaCodecPlayer
This is a prerequisite for MediaCodecPlayer (to be committed later).
Each decoder controls the queue of incoming access units and the
Android MediaCodec object through the instance of MediaCodecBridge.
The decoder owns a dedicated thread (Decoder thread) to access the
MediaCodecBridge. The manipulations with MediaCodec buffers, both
input and output, as well the rendering happen on this one thread.
For discussion see https://codereview.chromium.org/1128383003/
BUG=407577
Committed: https://crrev.com/d472a7860ac521ef72c53c9bb04926d4747e0b01
Cr-Commit-Position: refs/heads/master@{#335618}
Committed: https://crrev.com/7c32aea5c6fa487628501ac6dff4dddcce623152
Cr-Commit-Position: refs/heads/master@{#335849}
Patch Set 1 #Patch Set 2 : Added tests for video decoder, video decoder now reports current time. #
Total comments: 10
Patch Set 3 : Rebased to include AccessUnitQueue. #Patch Set 4 : Added kPrefetched and kError states, RequestToStop works in any state. #Patch Set 5 : Refactored DataFactory into a separate file #
Total comments: 16
Patch Set 6 : Addressed Min's comments #
Total comments: 47
Patch Set 7 : Addressed most of wolenetz@ comments #Patch Set 8 : Removed weak_ptr_ member variable #
Total comments: 9
Patch Set 9 : Fixed DemuxerConfigs logging functions #
Total comments: 1
Patch Set 10 : Another logging fix #Patch Set 11 : Fixed clang compilation, removed refcounts from TestDataFactory #Patch Set 12 : Fixed unit tests on Nexus 4 #Patch Set 13 : A better unittest fix, increased timeout for video #Messages
Total messages: 49 (10 generated)
timav@chromium.org changed reviewers: + qinmin@chromium.org, wolenetz@chromium.org
This is the decoder split off the original MediaCodecPlayer CL. I added the beginning of unit tests.
timav@chromium.org changed reviewers: + liberato@chromium.org, watk@chromium.org
On 2015/06/11 17:23:06, Tima Vaisburd wrote: Following CR of prereq CL and a couple CRs, I'll get to this review tomorrow. I was in training half of today.
On 2015/06/12 00:04:26, wolenetz wrote: > On 2015/06/11 17:23:06, Tima Vaisburd wrote: > > Following CR of prereq CL and a couple CRs, I'll get to this review tomorrow. I > was in training half of today. I felt this CL can be a moving target until the prerequisite (AccessUnitQueue) commits. Sorry if my change caught you in the middle of the review.
https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:16: AccessUnitQueue::~AccessUnitQueue() { nit: why provide an empty, non-virtual destructor? probably doesn't make a difference since the class has members which (probably) have non-trivial destructors. AUQ will end up with the same non-trivial destructor either way. or is there some rule i'm forgetting about these things? c++ is so subtle these days, it's hard to tell. https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... media/base/android/media_codec_decoder.cc:379: media_task_runner_->PostTask(FROM_HERE, starvation_cb_); does |state_| need to be reset here, so that Start() doesn't return immediately? https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:24: const base::Closure& error_cb, when the error cb is called, what sort of action is the receiver supposed to take? is there cleanup? some documentation here (along with the other states) would help a lot. https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:24: const base::Closure& error_cb, there are five error calls, but the callback has no idea which of them occurred.
https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:16: AccessUnitQueue::~AccessUnitQueue() { On 2015/06/12 22:24:15, liberato wrote: > nit: why provide an empty, non-virtual destructor? probably doesn't make a > difference since the class has members which (probably) have non-trivial > destructors. AUQ will end up with the same non-trivial destructor either way. > > or is there some rule i'm forgetting about these things? c++ is so subtle these > days, it's hard to tell. Having access_unit_queue in this CL is really confusing. The queue and it's unit tests are extracted and iterating in a separate CL with Matt and is supposed to land first. I had to put an obsolete copy of these files to make this CL compile and work.
On 2015/06/12 22:49:02, Tima Vaisburd wrote: > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... > File media/base/android/access_unit_queue.cc (right): > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... > media/base/android/access_unit_queue.cc:16: AccessUnitQueue::~AccessUnitQueue() > { > On 2015/06/12 22:24:15, liberato wrote: > > nit: why provide an empty, non-virtual destructor? probably doesn't make a > > difference since the class has members which (probably) have non-trivial > > destructors. AUQ will end up with the same non-trivial destructor either way. > > > > or is there some rule i'm forgetting about these things? c++ is so subtle > these > > days, it's hard to tell. > > Having access_unit_queue in this CL is really confusing. The queue and it's unit > tests are extracted and iterating in a separate CL with Matt and is supposed to > land first. I had to put an obsolete copy of these files to make this CL compile > and work. In future, it can help to isolate a chain of dependent CLs' reviews from each other somewhat by having each dependent CL in the chain be a branch off the prereq's CL branch in your workspace. e.g. origin/master <---- CL A branch with commit(s) <---- CL B branch with commit(s) Just like rebasing A against origin/master, when B needs to rebase, rebase it against A. e.g. git checkout origin/master git checkout -b CL_A # initial branch to work on CL_A # make A git commit # commit on CL_A git cl upload # etc... git checkout -b CL_B # initial branch to work on CL_B, and depends on CL_A git branch --set-upstream-to=CL_A # explicitly make sure that CL_B is based on CL_A # make B git commit # commit on CL_B git cl upload # etc... If a new commit lands in CL_A, to make sure CL_B is coherent, rebase CL_B onto that CL_A HEAD. git cl / Rietveld will then show for CL_B just the diff versus the CL_A prereq. Make sure you comment in CL_B Rietveld that CL_A is depended upon to land first. I hope this helps at least in future :) Have a great weekend!
On 2015/06/12 23:00:58, wolenetz wrote: > On 2015/06/12 22:49:02, Tima Vaisburd wrote: > > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... > > File media/base/android/access_unit_queue.cc (right): > > > > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... > > media/base/android/access_unit_queue.cc:16: > AccessUnitQueue::~AccessUnitQueue() > > { > > On 2015/06/12 22:24:15, liberato wrote: > > > nit: why provide an empty, non-virtual destructor? probably doesn't make a > > > difference since the class has members which (probably) have non-trivial > > > destructors. AUQ will end up with the same non-trivial destructor either > way. > > > > > > or is there some rule i'm forgetting about these things? c++ is so subtle > > these > > > days, it's hard to tell. > > > > Having access_unit_queue in this CL is really confusing. The queue and it's > unit > > tests are extracted and iterating in a separate CL with Matt and is supposed > to > > land first. I had to put an obsolete copy of these files to make this CL > compile > > and work. > > In future, it can help to isolate a chain of dependent CLs' reviews from each > other somewhat by having each dependent CL in the chain be a branch off the > prereq's CL branch in your workspace. > e.g. > origin/master <---- CL A branch with commit(s) <---- CL B branch with commit(s) > Just like rebasing A against origin/master, when B needs to rebase, rebase it > against A. > e.g. > git checkout origin/master > git checkout -b CL_A # initial branch to work on CL_A > # make A > git commit # commit on CL_A > git cl upload # etc... > git checkout -b CL_B # initial branch to work on CL_B, and depends on CL_A > git branch --set-upstream-to=CL_A # explicitly make sure that CL_B is based on > CL_A > # make B > git commit # commit on CL_B > git cl upload # etc... > > If a new commit lands in CL_A, to make sure CL_B is coherent, rebase CL_B onto > that CL_A HEAD. > > git cl / Rietveld will then show for CL_B just the diff versus the CL_A prereq. > Make sure you comment in CL_B Rietveld that CL_A is depended upon to land first. > > I hope this helps at least in future :) > Have a great weekend! That's cool, thank you!
On 2015/06/15 18:06:30, Tima Vaisburd wrote: > On 2015/06/12 23:00:58, wolenetz wrote: > > On 2015/06/12 22:49:02, Tima Vaisburd wrote: > > > > > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... > > > File media/base/android/access_unit_queue.cc (right): > > > > > > > > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... > > > media/base/android/access_unit_queue.cc:16: > > AccessUnitQueue::~AccessUnitQueue() > > > { > > > On 2015/06/12 22:24:15, liberato wrote: > > > > nit: why provide an empty, non-virtual destructor? probably doesn't make > a > > > > difference since the class has members which (probably) have non-trivial > > > > destructors. AUQ will end up with the same non-trivial destructor either > > way. > > > > > > > > or is there some rule i'm forgetting about these things? c++ is so subtle > > > these > > > > days, it's hard to tell. > > > > > > Having access_unit_queue in this CL is really confusing. The queue and it's > > unit > > > tests are extracted and iterating in a separate CL with Matt and is supposed > > to > > > land first. I had to put an obsolete copy of these files to make this CL > > compile > > > and work. > > > > In future, it can help to isolate a chain of dependent CLs' reviews from each > > other somewhat by having each dependent CL in the chain be a branch off the > > prereq's CL branch in your workspace. > > e.g. > > origin/master <---- CL A branch with commit(s) <---- CL B branch with > commit(s) > > Just like rebasing A against origin/master, when B needs to rebase, rebase it > > against A. > > e.g. > > git checkout origin/master > > git checkout -b CL_A # initial branch to work on CL_A > > # make A > > git commit # commit on CL_A > > git cl upload # etc... > > git checkout -b CL_B # initial branch to work on CL_B, and depends on CL_A > > git branch --set-upstream-to=CL_A # explicitly make sure that CL_B is based on > > CL_A > > # make B > > git commit # commit on CL_B > > git cl upload # etc... > > > > If a new commit lands in CL_A, to make sure CL_B is coherent, rebase CL_B onto > > that CL_A HEAD. > > > > git cl / Rietveld will then show for CL_B just the diff versus the CL_A > prereq. > > Make sure you comment in CL_B Rietveld that CL_A is depended upon to land > first. > > > > I hope this helps at least in future :) > > Have a great weekend! > > That's cool, thank you! Now that the AccessUnitQueue prerequisite CL has landed, please upload a rebased CL for this review.
On 2015/06/15 18:34:17, wolenetz wrote: > On 2015/06/15 18:06:30, Tima Vaisburd wrote: > > On 2015/06/12 23:00:58, wolenetz wrote: > > > On 2015/06/12 22:49:02, Tima Vaisburd wrote: > > > > > > > > > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... > > > > File media/base/android/access_unit_queue.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... > > > > media/base/android/access_unit_queue.cc:16: > > > AccessUnitQueue::~AccessUnitQueue() > > > > { > > > > On 2015/06/12 22:24:15, liberato wrote: > > > > > nit: why provide an empty, non-virtual destructor? probably doesn't > make > > a > > > > > difference since the class has members which (probably) have non-trivial > > > > > destructors. AUQ will end up with the same non-trivial destructor > either > > > way. > > > > > > > > > > or is there some rule i'm forgetting about these things? c++ is so > subtle > > > > these > > > > > days, it's hard to tell. > > > > > > > > Having access_unit_queue in this CL is really confusing. The queue and > it's > > > unit > > > > tests are extracted and iterating in a separate CL with Matt and is > supposed > > > to > > > > land first. I had to put an obsolete copy of these files to make this CL > > > compile > > > > and work. > > > > > > In future, it can help to isolate a chain of dependent CLs' reviews from > each > > > other somewhat by having each dependent CL in the chain be a branch off the > > > prereq's CL branch in your workspace. > > > e.g. > > > origin/master <---- CL A branch with commit(s) <---- CL B branch with > > commit(s) > > > Just like rebasing A against origin/master, when B needs to rebase, rebase > it > > > against A. > > > e.g. > > > git checkout origin/master > > > git checkout -b CL_A # initial branch to work on CL_A > > > # make A > > > git commit # commit on CL_A > > > git cl upload # etc... > > > git checkout -b CL_B # initial branch to work on CL_B, and depends on CL_A > > > git branch --set-upstream-to=CL_A # explicitly make sure that CL_B is based > on > > > CL_A > > > # make B > > > git commit # commit on CL_B > > > git cl upload # etc... > > > > > > If a new commit lands in CL_A, to make sure CL_B is coherent, rebase CL_B > onto > > > that CL_A HEAD. > > > > > > git cl / Rietveld will then show for CL_B just the diff versus the CL_A > > prereq. > > > Make sure you comment in CL_B Rietveld that CL_A is depended upon to land > > first. > > > > > > I hope this helps at least in future :) > > > Have a great weekend! > > > > That's cool, thank you! > > Now that the AccessUnitQueue prerequisite CL has landed, please upload a rebased > CL for this review. Sure, I'm in the process of doing that. The git scheme would be just one branch against master, not the one you described - that one would be great for the player CL, I think.
https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:16: AccessUnitQueue::~AccessUnitQueue() { On 2015/06/12 22:49:01, Tima Vaisburd wrote: > On 2015/06/12 22:24:15, liberato wrote: > > nit: why provide an empty, non-virtual destructor? probably doesn't make a > > difference since the class has members which (probably) have non-trivial > > destructors. AUQ will end up with the same non-trivial destructor either way. > > > > or is there some rule i'm forgetting about these things? c++ is so subtle > these > > days, it's hard to tell. > > Having access_unit_queue in this CL is really confusing. The queue and it's unit > tests are extracted and iterating in a separate CL with Matt and is supposed to > land first. I had to put an obsolete copy of these files to make this CL compile > and work. In the latest version of AccessUnitQueue that has been committed destructor became non-trivial. Back to the essence of you question though: I put the destructor by habit, although maybe a bad one... My major motivation was to explicitly show that "destructor is here and is default desctructor, indeed". Omitting it would do just the same, and I do not know any c++ rule that would overcome this, I think you are absolutely right. You would need to make sure there is no dtor at all by examining the header file, which might be inconvenient as the class grows bigger. Probably "=default" from c++11 is the best. https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... media/base/android/media_codec_decoder.cc:379: media_task_runner_->PostTask(FROM_HERE, starvation_cb_); On 2015/06/12 22:24:15, liberato wrote: > does |state_| need to be reset here, so that Start() doesn't return immediately? I'm not quite sure I completely understood your idea. Right now the decoder report the starvation, but it state does not change, and the thread is not stopped either. The player responds by async stopping all decoders, then going to prefetch, and then start. During the process of async stop the decoder state will change. I see another way of doing this: decoder stops itself upon detecting starvation (and correspondingly changes its state). The player would need to async stop other decoders, or decoder's async stop should support multiple calls in a row. Do you say that the second way seems cleaner?
On 2015/06/15 18:43:31, Tima Vaisburd wrote: > On 2015/06/15 18:34:17, wolenetz wrote: > > On 2015/06/15 18:06:30, Tima Vaisburd wrote: > > > On 2015/06/12 23:00:58, wolenetz wrote: > > > > On 2015/06/12 22:49:02, Tima Vaisburd wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... > > > > > File media/base/android/access_unit_queue.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... > > > > > media/base/android/access_unit_queue.cc:16: > > > > AccessUnitQueue::~AccessUnitQueue() > > > > > { > > > > > On 2015/06/12 22:24:15, liberato wrote: > > > > > > nit: why provide an empty, non-virtual destructor? probably doesn't > > make > > > a > > > > > > difference since the class has members which (probably) have > non-trivial > > > > > > destructors. AUQ will end up with the same non-trivial destructor > > either > > > > way. > > > > > > > > > > > > or is there some rule i'm forgetting about these things? c++ is so > > subtle > > > > > these > > > > > > days, it's hard to tell. > > > > > > > > > > Having access_unit_queue in this CL is really confusing. The queue and > > it's > > > > unit > > > > > tests are extracted and iterating in a separate CL with Matt and is > > supposed > > > > to > > > > > land first. I had to put an obsolete copy of these files to make this CL > > > > compile > > > > > and work. > > > > > > > > In future, it can help to isolate a chain of dependent CLs' reviews from > > each > > > > other somewhat by having each dependent CL in the chain be a branch off > the > > > > prereq's CL branch in your workspace. > > > > e.g. > > > > origin/master <---- CL A branch with commit(s) <---- CL B branch with > > > commit(s) > > > > Just like rebasing A against origin/master, when B needs to rebase, rebase > > it > > > > against A. > > > > e.g. > > > > git checkout origin/master > > > > git checkout -b CL_A # initial branch to work on CL_A > > > > # make A > > > > git commit # commit on CL_A > > > > git cl upload # etc... > > > > git checkout -b CL_B # initial branch to work on CL_B, and depends on CL_A > > > > git branch --set-upstream-to=CL_A # explicitly make sure that CL_B is > based > > on > > > > CL_A > > > > # make B > > > > git commit # commit on CL_B > > > > git cl upload # etc... > > > > > > > > If a new commit lands in CL_A, to make sure CL_B is coherent, rebase CL_B > > onto > > > > that CL_A HEAD. > > > > > > > > git cl / Rietveld will then show for CL_B just the diff versus the CL_A > > > prereq. > > > > Make sure you comment in CL_B Rietveld that CL_A is depended upon to land > > > first. > > > > > > > > I hope this helps at least in future :) > > > > Have a great weekend! > > > > > > That's cool, thank you! > > > > Now that the AccessUnitQueue prerequisite CL has landed, please upload a > rebased > > CL for this review. > > Sure, I'm in the process of doing that. The git scheme would be just one branch > against master, > not the one you described - that one would be great for the player CL, I think. Yes. My earlier help was for simplifying review for such a scenario as master(withAUQueueLanded) <--- ThisCodecsCL <---- SomeOtherPlayerCL dependency chain.
On 2015/06/15 19:10:24, wolenetz wrote: > On 2015/06/15 18:43:31, Tima Vaisburd wrote: > > On 2015/06/15 18:34:17, wolenetz wrote: > > > On 2015/06/15 18:06:30, Tima Vaisburd wrote: > > > > On 2015/06/12 23:00:58, wolenetz wrote: > > > > > On 2015/06/12 22:49:02, Tima Vaisburd wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... > > > > > > File media/base/android/access_unit_queue.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/acce... > > > > > > media/base/android/access_unit_queue.cc:16: > > > > > AccessUnitQueue::~AccessUnitQueue() > > > > > > { > > > > > > On 2015/06/12 22:24:15, liberato wrote: > > > > > > > nit: why provide an empty, non-virtual destructor? probably doesn't > > > make > > > > a > > > > > > > difference since the class has members which (probably) have > > non-trivial > > > > > > > destructors. AUQ will end up with the same non-trivial destructor > > > either > > > > > way. > > > > > > > > > > > > > > or is there some rule i'm forgetting about these things? c++ is so > > > subtle > > > > > > these > > > > > > > days, it's hard to tell. > > > > > > > > > > > > Having access_unit_queue in this CL is really confusing. The queue and > > > it's > > > > > unit > > > > > > tests are extracted and iterating in a separate CL with Matt and is > > > supposed > > > > > to > > > > > > land first. I had to put an obsolete copy of these files to make this > CL > > > > > compile > > > > > > and work. > > > > > > > > > > In future, it can help to isolate a chain of dependent CLs' reviews from > > > each > > > > > other somewhat by having each dependent CL in the chain be a branch off > > the > > > > > prereq's CL branch in your workspace. > > > > > e.g. > > > > > origin/master <---- CL A branch with commit(s) <---- CL B branch with > > > > commit(s) > > > > > Just like rebasing A against origin/master, when B needs to rebase, > rebase > > > it > > > > > against A. > > > > > e.g. > > > > > git checkout origin/master > > > > > git checkout -b CL_A # initial branch to work on CL_A > > > > > # make A > > > > > git commit # commit on CL_A > > > > > git cl upload # etc... > > > > > git checkout -b CL_B # initial branch to work on CL_B, and depends on > CL_A > > > > > git branch --set-upstream-to=CL_A # explicitly make sure that CL_B is > > based > > > on > > > > > CL_A > > > > > # make B > > > > > git commit # commit on CL_B > > > > > git cl upload # etc... > > > > > > > > > > If a new commit lands in CL_A, to make sure CL_B is coherent, rebase > CL_B > > > onto > > > > > that CL_A HEAD. > > > > > > > > > > git cl / Rietveld will then show for CL_B just the diff versus the CL_A > > > > prereq. > > > > > Make sure you comment in CL_B Rietveld that CL_A is depended upon to > land > > > > first. > > > > > > > > > > I hope this helps at least in future :) > > > > > Have a great weekend! > > > > > > > > That's cool, thank you! > > > > > > Now that the AccessUnitQueue prerequisite CL has landed, please upload a > > rebased > > > CL for this review. > > > > Sure, I'm in the process of doing that. The git scheme would be just one > branch > > against master, > > not the one you described - that one would be great for the player CL, I > think. > > Yes. My earlier help was for simplifying review for such a scenario as > master(withAUQueueLanded) <--- ThisCodecsCL <---- SomeOtherPlayerCL dependency > chain. Done.
https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:24: const base::Closure& error_cb, On 2015/06/12 22:24:15, liberato wrote: > there are five error calls, but the callback has no idea which of them occurred. Since there is no way to recover from these errors other than calling ReleaseDecoderResources() I did not provide additional details (see my other comment about the recovery). How do you think this info can be useful for the upper level? https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:24: const base::Closure& error_cb, On 2015/06/12 22:24:15, liberato wrote: > when the error cb is called, what sort of action is the receiver supposed to > take? is there cleanup? some documentation here (along with the other states) > would help a lot. Good point, I updated the documentation. The recovery is supposed to be done by upper layer with ResetDecoderResources(), which releases the MediaCodec. I'm not sure what else could be done. I added a new kError state to block Prefetch/Configure/Start if we got an error.
https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:24: const base::Closure& error_cb, On 2015/06/17 01:18:24, Tima Vaisburd wrote: > On 2015/06/12 22:24:15, liberato wrote: > > when the error cb is called, what sort of action is the receiver supposed to > > take? is there cleanup? some documentation here (along with the other > states) > > would help a lot. > > Good point, I updated the documentation. The recovery is supposed to be done by > upper layer with ResetDecoderResources(), which releases the MediaCodec. I'm not > sure what else could be done. I added a new kError state to block > Prefetch/Configure/Start if we got an error. Sounds like it was just a doc issue. If there's no action to take, then i agree that additional info isn't useful. thanks for clarifying.
Thank you. What else needs to be done wrt this CL? I haven't received approval from any reviewer which might be an indication of a bigger problem. On Wed, Jun 17, 2015 at 7:11 AM, <liberato@chromium.org> wrote: > > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... > File media/base/android/media_codec_video_decoder.cc (right): > > > https://codereview.chromium.org/1176993005/diff/20001/media/base/android/medi... > media/base/android/media_codec_video_decoder.cc:24: const base::Closure& > error_cb, > On 2015/06/17 01:18:24, Tima Vaisburd wrote: > >> On 2015/06/12 22:24:15, liberato wrote: >> > when the error cb is called, what sort of action is the receiver >> > supposed to > >> > take? is there cleanup? some documentation here (along with the >> > other > >> states) >> > would help a lot. >> > > Good point, I updated the documentation. The recovery is supposed to >> > be done by > >> upper layer with ResetDecoderResources(), which releases the >> > MediaCodec. I'm not > >> sure what else could be done. I added a new kError state to block >> Prefetch/Configure/Start if we got an error. >> > > Sounds like it was just a doc issue. If there's no action to take, then > i agree that additional info isn't useful. thanks for clarifying. > > https://codereview.chromium.org/1176993005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.cc:132: // drm_bridge_ is not implemented add a TODO https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.cc:373: if (NumDelayedRenderTasks() <= 1) { make this a one line statement: if (NumDelayedRenderTasks() <= 1 && !EnqueueInputBuffer()) https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... File media/base/android/media_codec_decoder_unittest.cc (right): https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder_unittest.cc:200: } this can go to the previous line https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:94: curr.is_video_encrypted != next.is_video_encrypted) nit: {} needed for if statement that spans multiple lines https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:100: curr.video_size.height() == next.video_size.height()) ditto https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:124: // drm_bridge()->IsProtectedSurfaceRequired(); file a crbug and add TODO here. https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:170: if (video_size_ != prev_size) {} needed https://codereview.chromium.org/1176993005/diff/80001/media/base/android/test... File media/base/android/test_data_factory.h (right): https://codereview.chromium.org/1176993005/diff/80001/media/base/android/test... media/base/android/test_data_factory.h:38: virtual ~TestDataFactory() {} nit: put this to the .cc file
I also added description for the audio and video decoder constructor parameters. https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.cc:132: // drm_bridge_ is not implemented On 2015/06/18 00:24:40, qinmin wrote: > add a TODO Done. https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder.cc:373: if (NumDelayedRenderTasks() <= 1) { On 2015/06/18 00:24:40, qinmin wrote: > make this a one line statement: > if (NumDelayedRenderTasks() <= 1 && !EnqueueInputBuffer()) I put the NumDelayedRenderTasks() condition inside the method EnqueueInputBuffer(): it returns early if the number exceeds the limit. I think it would be easier to log or debug this way. https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... File media/base/android/media_codec_decoder_unittest.cc (right): https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_decoder_unittest.cc:200: } On 2015/06/18 00:24:40, qinmin wrote: > this can go to the previous line Done. https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:94: curr.is_video_encrypted != next.is_video_encrypted) On 2015/06/18 00:24:40, qinmin wrote: > nit: {} needed for if statement that spans multiple lines Done. https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:100: curr.video_size.height() == next.video_size.height()) On 2015/06/18 00:24:40, qinmin wrote: > ditto Done. https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:124: // drm_bridge()->IsProtectedSurfaceRequired(); On 2015/06/18 00:24:40, qinmin wrote: > file a crbug and add TODO here. http://crbug.com/501943 https://codereview.chromium.org/1176993005/diff/80001/media/base/android/medi... media/base/android/media_codec_video_decoder.cc:170: if (video_size_ != prev_size) On 2015/06/18 00:24:40, qinmin wrote: > {} needed Done. https://codereview.chromium.org/1176993005/diff/80001/media/base/android/test... File media/base/android/test_data_factory.h (right): https://codereview.chromium.org/1176993005/diff/80001/media/base/android/test... media/base/android/test_data_factory.h:38: virtual ~TestDataFactory() {} On 2015/06/18 00:24:40, qinmin wrote: > nit: put this to the .cc file Done.
lgtm % nit https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:106: ->IsAdaptivePlaybackSupported(next.video_size.width(), nit: fix the indent
Some comments on PS6. I'll continue review beyond the base decoder class review, next. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/dem... File media/base/android/demuxer_stream_player_params.cc (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:33: << (au.is_key_frame ? " KEY_FRAME" : "") << " pts:" << au.timestamp nit: it might also be interesting to output dts here as well, since pts isn't guaranteed to be monotonically non-decreasing (like dts) in out-of-order decode codecs (like h264). https://codereview.chromium.org/1176993005/diff/100001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:34: << " size:" << au.data.size(); nit: if an AU contains a config change, that would be especially interesting data to log. Please add that here, too. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:35: // Decoder's internal state machine goes through the following states: nit: what transition(s) may Flush() occur on? Or does Flush not change state? Is Flush disallowed in some states? https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:39: // | Prefetch |--- internal ------| nit: I'm not sure from this diagram (though, thanks for adding it!!): does ReleaseDecoderResources also happen on a regular (not with a transition through Error) transition from Stopping to Stopped? https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:45: // [ Prefetched ] | [ Stopped ] nit: it appears the error->stopped->prefetching path is allowed. Is that expected? https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:64: // The decoder reports current playback time to the MediaCodecPlayer. please document the two TimeDelta parameters https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:83: // Called when an unrecoverable MediaCodec error occurred. If this happen, nit: s/happen/happens/ https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:85: // decoder object. nit: (see diagram comment): is error truly unrecoverable if transition from Error through Stopped allows restart by Prefetch? https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:255: // Flag is set when the EOS is enqueued into MediaCodec. Is EOS reset by Flush or some other operation? e.g. Player could seek to escape EOS, or web app might make more data available to be demuxed and continue playback to escape 'ended' state. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:258: // Flag is set when the EOS is received in MediaCodec output. ditto: when might this flag be reset? https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:264: base::WeakPtr<MediaCodecDecoder> weak_this_; I'm confused: do we really need both weak_this_ and weak_factory_?
Overall, this is looking pretty good. I especially like the breaking apart of the testfactory helper to make them more readable. I've completed PS6 review pass. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.cc:85: DCHECK_EQ(GetState(), kStopped); nit: (not strong feeling here) Why not just DCHECK(IsStopped()) here and elsewhere? https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.cc:162: // Here I assume that OnDemuxerConfigsAvailable won't come aside: This is probably a valid assumption. (Configs would need to be explicitly requested, first.) Is there some way to check this situation? https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.cc:392: base::TimeDelta::FromMilliseconds(kNextFrameDelay)); aside: might this prevent (really high) high frame rate playback from being reliable? I hope not, but ... https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.cc:497: bool MediaCodecDecoder::DepleteOutputBufferQueue(bool* eos_encountered) { aside: I like this naming :) https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:119: // The processing of CONFIG_KEY_FRAME_REQUIRED is not implemented yet, nit: add TODO / bug? (is this something the player would handle?) https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:189: // EOS might come as a stand-alone frame with zero size in what case is the media_source_delegate (or chunk_demuxer) emitting such a strange EOS? If that's happening, seems it might be a bug there on the demuxer to detect this EOS condition sooner. If not, then this workaround isn't needed.
I hope I addressed all of Matt's comments (there are some minor questions) except checking the assumption in Configure() which I do not know how to do. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/dem... File media/base/android/demuxer_stream_player_params.cc (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:33: << (au.is_key_frame ? " KEY_FRAME" : "") << " pts:" << au.timestamp On 2015/06/19 22:46:42, wolenetz wrote: > nit: it might also be interesting to output dts here as well, since pts isn't > guaranteed to be monotonically non-decreasing (like dts) in out-of-order decode > codecs (like h264). Yes, I even have this situation in the unit test :-) There is only one |timestamp| field in AccessUnit and I believe this is PTS. Although there is no DTS, I think we know the decoding order, which is the order the AU are coming from demuxer. At least this is the order I put them into MediaCodec, so I made this assumption. For the logs this order comes naturally as I print the chunk one AU after another. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:34: << " size:" << au.data.size(); On 2015/06/19 22:46:41, wolenetz wrote: > nit: if an AU contains a config change, that would be especially interesting > data to log. Please add that here, too. I added operator<< but used is so far in SetDemuxerConfigs only. Unfortunately there is a duplication, could not use https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_d... (and video) because they are not static. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.cc:85: DCHECK_EQ(GetState(), kStopped); On 2015/06/19 23:28:41, wolenetz wrote: > nit: (not strong feeling here) Why not just DCHECK(IsStopped()) here and > elsewhere? When I was writing this I imagined that IsStopped() is a public method to be used by external controller (i.e. player). I can change if you say... https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.cc:392: base::TimeDelta::FromMilliseconds(kNextFrameDelay)); On 2015/06/19 23:28:41, wolenetz wrote: > aside: might this prevent (really high) high frame rate playback from being > reliable? I hope not, but ... I haven't seen this so far. I also reduced kNextFrameDelay to 1 ms, should be as good as 2 ms. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.cc:497: bool MediaCodecDecoder::DepleteOutputBufferQueue(bool* eos_encountered) { On 2015/06/19 23:28:41, wolenetz wrote: > aside: I like this naming :) M-mm.. wrong choice? I meant "process till the queue is empty". https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:35: // Decoder's internal state machine goes through the following states: On 2015/06/19 22:46:42, wolenetz wrote: > nit: what transition(s) may Flush() occur on? Or does Flush not change state? Is > Flush disallowed in some states? Flush is only allowed in the Stopped state. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:45: // [ Prefetched ] | [ Stopped ] On 2015/06/19 22:46:42, wolenetz wrote: > nit: it appears the error->stopped->prefetching path is allowed. Is that > expected? Yes, after ReleaseDecoderResources on the next cycle we will be creating a new MediaCodec object. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:45: // [ Prefetched ] | [ Stopped ] On 2015/06/19 22:46:42, wolenetz wrote: > nit: it appears the error->stopped->prefetching path is allowed. Is that > expected? Yes, ReleaseDecoderResources is a path to recovery. I changed the diagram slightly, is it more clear now? https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:64: // The decoder reports current playback time to the MediaCodecPlayer. On 2015/06/19 22:46:42, wolenetz wrote: > please document the two TimeDelta parameters Done. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:83: // Called when an unrecoverable MediaCodec error occurred. If this happen, On 2015/06/19 22:46:42, wolenetz wrote: > nit: s/happen/happens/ Done. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:85: // decoder object. On 2015/06/19 22:46:42, wolenetz wrote: > nit: (see diagram comment): is error truly unrecoverable if transition from > Error through Stopped allows restart by Prefetch? Removed word "unrecoverable". https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:255: // Flag is set when the EOS is enqueued into MediaCodec. On 2015/06/19 22:46:42, wolenetz wrote: > Is EOS reset by Flush or some other operation? e.g. Player could seek to escape > EOS, or web app might make more data available to be demuxed and continue > playback to escape 'ended' state. Added "Reset by Flush" in the comment. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:258: // Flag is set when the EOS is received in MediaCodec output. On 2015/06/19 22:46:42, wolenetz wrote: > ditto: when might this flag be reset? Same thing, added "Reset by Flush" in the comment. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:264: base::WeakPtr<MediaCodecDecoder> weak_this_; On 2015/06/19 22:46:42, wolenetz wrote: > I'm confused: do we really need both weak_this_ and weak_factory_? I copied this pattern from MedisSourcePlayer. I thought it is to make the code more concise. Would you like me to change it to factory.GetWeakPtr() everywhere? https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:106: ->IsAdaptivePlaybackSupported(next.video_size.width(), On 2015/06/19 06:57:40, qinmin wrote: > nit: fix the indent I did although this was against |git cl format|. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:119: // The processing of CONFIG_KEY_FRAME_REQUIRED is not implemented yet, On 2015/06/19 23:28:41, wolenetz wrote: > nit: add TODO / bug? (is this something the player would handle?) Added TODO. This should be a part of the browser seek. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:189: // EOS might come as a stand-alone frame with zero size On 2015/06/19 23:28:41, wolenetz wrote: > in what case is the media_source_delegate (or chunk_demuxer) emitting such a > strange EOS? If that's happening, seems it might be a bug there on the demuxer > to detect this EOS condition sooner. If not, then this workaround isn't needed. MediaCodecBridge::QueueEOS generates these frames itself: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... We use it now and will use for one extra case to drain decoder as a part of dynamic reconfiguration process.
https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:264: base::WeakPtr<MediaCodecDecoder> weak_this_; On 2015/06/20 02:32:31, Tima Vaisburd wrote: > On 2015/06/19 22:46:42, wolenetz wrote: > > I'm confused: do we really need both weak_this_ and weak_factory_? > > I copied this pattern from MedisSourcePlayer. I thought it is to make the code > more concise. Would you like me to change it to factory.GetWeakPtr() everywhere? Removed weak_this_ in favor of GetWeakPtr(). https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:189: // EOS might come as a stand-alone frame with zero size On 2015/06/20 02:32:31, Tima Vaisburd wrote: > On 2015/06/19 23:28:41, wolenetz wrote: > > in what case is the media_source_delegate (or chunk_demuxer) emitting such a > > strange EOS? If that's happening, seems it might be a bug there on the demuxer > > to detect this EOS condition sooner. If not, then this workaround isn't > needed. > > MediaCodecBridge::QueueEOS generates these frames itself: > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... > We use it now and will use for one extra case to drain decoder as a part of > dynamic reconfiguration process. I checked again, this kind of EOS seems to always come from chunk demuxer. Maybe I did not formulate it properly: by stand-alone I meant the access unit with flag(s), but no data. From what I've seen so far, the access unit that holds EOS has neither data nor PTS. The android MediaCodec always assigns PTS to the output buffer though such that the next PTS is equal or greater than previous. I changed the comment, hope it became clearer.
Just a couple further issues (demuxer_stream_player logging stuff). Otherwise, l*tm. watk@, did you also want to take a look? https://codereview.chromium.org/1176993005/diff/100001/media/base/android/dem... File media/base/android/demuxer_stream_player_params.cc (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:33: << (au.is_key_frame ? " KEY_FRAME" : "") << " pts:" << au.timestamp On 2015/06/20 02:32:30, Tima Vaisburd wrote: > On 2015/06/19 22:46:42, wolenetz wrote: > > nit: it might also be interesting to output dts here as well, since pts isn't > > guaranteed to be monotonically non-decreasing (like dts) in out-of-order > decode > > codecs (like h264). > > Yes, I even have this situation in the unit test :-) > > There is only one |timestamp| field in AccessUnit and I believe this is PTS. > Although there is no DTS, I think we know the decoding order, which is the order > the AU are coming from demuxer. At least this is the order I put them into > MediaCodec, so I made this assumption. > For the logs this order comes naturally as I print the chunk one AU after > another. You're right. Ignore my comment - we don't need to add DTS to AU just for logging, nor in this CL. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:34: << " size:" << au.data.size(); On 2015/06/20 02:32:30, Tima Vaisburd wrote: > On 2015/06/19 22:46:41, wolenetz wrote: > > nit: if an AU contains a config change, that would be especially interesting > > data to log. Please add that here, too. > > I added operator<< but used is so far in SetDemuxerConfigs only. Unfortunately > there is a duplication, could not use > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_d... > (and video) because they are not static. Duplication is ugly; but unless we jump forward to a mojo-ized world where we deserialize back into a {Audio,Video}DecoderConfig, then we'll use what you have here. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... File media/base/android/media_codec_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.cc:85: DCHECK_EQ(GetState(), kStopped); On 2015/06/20 02:32:30, Tima Vaisburd wrote: > On 2015/06/19 23:28:41, wolenetz wrote: > > nit: (not strong feeling here) Why not just DCHECK(IsStopped()) here and > > elsewhere? > > When I was writing this I imagined that IsStopped() is a public method to be > used by external controller (i.e. player). I can change if you say... no strong feeling. as-is is fine. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.cc:392: base::TimeDelta::FromMilliseconds(kNextFrameDelay)); On 2015/06/20 02:32:30, Tima Vaisburd wrote: > On 2015/06/19 23:28:41, wolenetz wrote: > > aside: might this prevent (really high) high frame rate playback from being > > reliable? I hope not, but ... > > I haven't seen this so far. I also reduced kNextFrameDelay to 1 ms, should be as > good as 2 ms. Acknowledged. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.cc:497: bool MediaCodecDecoder::DepleteOutputBufferQueue(bool* eos_encountered) { On 2015/06/20 02:32:30, Tima Vaisburd wrote: > On 2015/06/19 23:28:41, wolenetz wrote: > > aside: I like this naming :) > > M-mm.. wrong choice? I meant "process till the queue is empty". Hmm. Deplete[...] is better IMO than Process[...]. The comment in .h makes intent clear enough. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... File media/base/android/media_codec_decoder.h (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:35: // Decoder's internal state machine goes through the following states: On 2015/06/20 02:32:31, Tima Vaisburd wrote: > On 2015/06/19 22:46:42, wolenetz wrote: > > nit: what transition(s) may Flush() occur on? Or does Flush not change state? > Is > > Flush disallowed in some states? > > Flush is only allowed in the Stopped state. Acknowledged. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:39: // | Prefetch |--- internal ------| On 2015/06/19 22:46:42, wolenetz wrote: > nit: I'm not sure from this diagram (though, thanks for adding it!!): does > ReleaseDecoderResources also happen on a regular (not with a transition through > Error) transition from Stopping to Stopped? Acknowledged. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:45: // [ Prefetched ] | [ Stopped ] On 2015/06/20 02:32:31, Tima Vaisburd wrote: > On 2015/06/19 22:46:42, wolenetz wrote: > > nit: it appears the error->stopped->prefetching path is allowed. Is that > > expected? > > Yes, ReleaseDecoderResources is a path to recovery. > I changed the diagram slightly, is it more clear now? Acknowledged. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_decoder.h:264: base::WeakPtr<MediaCodecDecoder> weak_this_; On 2015/06/21 22:35:20, Tima Vaisburd wrote: > On 2015/06/20 02:32:31, Tima Vaisburd wrote: > > On 2015/06/19 22:46:42, wolenetz wrote: > > > I'm confused: do we really need both weak_this_ and weak_factory_? > > > > I copied this pattern from MedisSourcePlayer. I thought it is to make the code > > more concise. Would you like me to change it to factory.GetWeakPtr() > everywhere? > > Removed weak_this_ in favor of GetWeakPtr(). Acknowledged. https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... File media/base/android/media_codec_video_decoder.cc (right): https://codereview.chromium.org/1176993005/diff/100001/media/base/android/med... media/base/android/media_codec_video_decoder.cc:189: // EOS might come as a stand-alone frame with zero size On 2015/06/21 22:35:20, Tima Vaisburd wrote: > On 2015/06/20 02:32:31, Tima Vaisburd wrote: > > On 2015/06/19 23:28:41, wolenetz wrote: > > > in what case is the media_source_delegate (or chunk_demuxer) emitting such a > > > strange EOS? If that's happening, seems it might be a bug there on the > demuxer > > > to detect this EOS condition sooner. If not, then this workaround isn't > > needed. > > > > MediaCodecBridge::QueueEOS generates these frames itself: > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... > > We use it now and will use for one extra case to drain decoder as a part of > > dynamic reconfiguration process. > > I checked again, this kind of EOS seems to always come from chunk demuxer. Maybe > I did not formulate it properly: by stand-alone I meant the access unit with > flag(s), but no data. From what I've seen so far, the access unit that holds EOS > has neither data nor PTS. The android MediaCodec always assigns PTS to the > output buffer though such that the next PTS is equal or greater than previous. > I changed the comment, hope it became clearer. Thanks for the detail. I had confused myself: ::Render() is on the output of a decode, not on the queuing of input (of course), hence this is really the only way currently of detecting EOS within Render. Sorry for the confusion, and thanks for clarifying in comment. https://codereview.chromium.org/1176993005/diff/140001/media/base/android/dem... File media/base/android/demuxer_stream_player_params.cc (right): https://codereview.chromium.org/1176993005/diff/140001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:34: const char* AsString(AudioCodec codec) { nit: should these two AsString be static (private to .cc file-scope)? https://codereview.chromium.org/1176993005/diff/140001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:51: default: Here and in the next switch, remove the default case, and add NOTREACHED() + trivial return NULL outside the switch. IOW, fail fast if enum is changed in {audio,video}_decoder_config.h, and give a fast fail on debug builds if there's been an IPC err. Please check that compile error occurs (on Android) in the case where an enum value is temporarily left out, to confirm my expectation that a compile err would result (similar to desktop). https://codereview.chromium.org/1176993005/diff/140001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:66: default: ditto https://codereview.chromium.org/1176993005/diff/140001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:85: if (conf.audio_codec != media::kUnknownAudioCodec) { nit: if kUnknownAudioCodec, that's interesting, too (e.g., if BOTH audio and video are unknown, that's definitely interesting to log here.) https://codereview.chromium.org/1176993005/diff/140001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:92: if (conf.video_codec != media::kUnknownVideoCodec) { nit ditto (kUnknownVideoCodec)
On 2015/06/22 21:41:22, wolenetz wrote: > Just a couple further issues (demuxer_stream_player logging stuff). Otherwise, > l*tm. > > watk@, did you also want to take a look? I'm fine with your review, Matt. Thanks!
https://codereview.chromium.org/1176993005/diff/140001/media/base/android/dem... File media/base/android/demuxer_stream_player_params.cc (right): https://codereview.chromium.org/1176993005/diff/140001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:34: const char* AsString(AudioCodec codec) { On 2015/06/22 21:41:22, wolenetz wrote: > nit: should these two AsString be static (private to .cc file-scope)? Done. https://codereview.chromium.org/1176993005/diff/140001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:51: default: On 2015/06/22 21:41:21, wolenetz wrote: > Here and in the next switch, remove the default case, and add NOTREACHED() + > trivial return NULL outside the switch. IOW, fail fast if enum is changed in > {audio,video}_decoder_config.h, and give a fast fail on debug builds if there's > been an IPC err. Please check that compile error occurs (on Android) in the case > where an enum value is temporarily left out, to confirm my expectation that a > compile err would result (similar to desktop). Done. Compilation fails, indeed, with "enumeration value not handled" https://codereview.chromium.org/1176993005/diff/140001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:66: default: On 2015/06/22 21:41:22, wolenetz wrote: > ditto Done. https://codereview.chromium.org/1176993005/diff/140001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:85: if (conf.audio_codec != media::kUnknownAudioCodec) { On 2015/06/22 21:41:22, wolenetz wrote: > nit: if kUnknownAudioCodec, that's interesting, too (e.g., if BOTH audio and > video are unknown, that's definitely interesting to log here.) I added the log if both are unknown. If at least one is known, only the known ones are printed.
lgtm % nit https://codereview.chromium.org/1176993005/diff/160001/media/base/android/dem... File media/base/android/demuxer_stream_player_params.cc (right): https://codereview.chromium.org/1176993005/diff/160001/media/base/android/dem... media/base/android/demuxer_stream_player_params.cc:87: os << conf.duration; nit (sorry, didn't see this earlier): os << "duration:" << conf.duration;
lgtm
lgtm
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org, wolenetz@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1176993005/#ps180001 (title: "Another logging fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1176993005/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org, wolenetz@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1176993005/#ps200001 (title: "Fixed clang compilation, removed refcounts from TestDataFactory")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1176993005/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/d472a7860ac521ef72c53c9bb04926d4747e0b01 Cr-Commit-Position: refs/heads/master@{#335618}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/1194423008/ by schenney@chromium.org. The reason for reverting is: Failing tests. Specifically, media_unittests and org.chromium.android_webview.test.MediaAccessPermissionRequestTest#testGrantAccess https://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28db....
timav@chromium.org changed reviewers: - liberato@chromium.org, watk@chromium.org, wolenetz@chromium.org
Min, could you, please, check that the revert of revert is done correctly?
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org, wolenetz@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1176993005/#ps240001 (title: "A better unittest fix, increased timeout for video")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1176993005/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/7c32aea5c6fa487628501ac6dff4dddcce623152 Cr-Commit-Position: refs/heads/master@{#335849} |
