Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2273273002:
Directly call ChunkDemuxer::Initialize completion callback. (Closed)
|
Created:
4 years, 3 months ago by alokp Modified:
4 years, 2 months ago CC:
blink-reviews, chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDirectly call ChunkDemuxer::Initialize completion callback.
ChunkDemuxer posts init_cb while calls DemuxerHost::OnDemuxerError
directly, which creates a race between the two. In some cases if
there is an error right after a successful intialization, the error
will be reported before init_cb has a chance to run which violates
upstream expectations.
BUG=633016
Committed: https://crrev.com/0c4e92a05559f716367fdef9928ae6df0110ff65
Cr-Commit-Position: refs/heads/master@{#423463}
Patch Set 1 #Patch Set 2 : update ChunkDemuxerTest #
Total comments: 12
Patch Set 3 : addressed comments #
Total comments: 3
Patch Set 4 : public mock method #
Total comments: 25
Patch Set 5 : rebase #Patch Set 6 : fixed typos #Patch Set 7 : rebase #Patch Set 8 : rebase #Patch Set 9 : removes comment #
Messages
Total messages: 35 (12 generated)
Description was changed from ========== Do not post init cb. BUG=633016 ========== to ========== Directly call ChunkDemuxer::Initialize completion callback. ChunkDemuxer posts init_cb while calls DemuxerHost::OnDemuxerError directly, which creates a race between the two. In some cases if there is an error right after a successful intialization, the error will be reported before init_cb has a chance to run which violates upstream expectations. BUG=633016 ==========
Description was changed from ========== Directly call ChunkDemuxer::Initialize completion callback. ChunkDemuxer posts init_cb while calls DemuxerHost::OnDemuxerError directly, which creates a race between the two. In some cases if there is an error right after a successful intialization, the error will be reported before init_cb has a chance to run which violates upstream expectations. BUG=633016 ========== to ========== Directly call ChunkDemuxer::Initialize completion callback. ChunkDemuxer posts init_cb while calls DemuxerHost::OnDemuxerError directly, which creates a race between the two. In some cases if there is an error right after a successful intialization, the error will be reported before init_cb has a chance to run which violates upstream expectations. BUG=633016 ==========
The CQ bit was checked by alokp@chromium.org to run a CQ dry run
alokp@chromium.org changed reviewers: + dalecurtis@chromium.org, xhwang@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:793: MOCK_METHOD1(DemuxerInitialized, void(PipelineStatus)); Converted InitDoneCalled into a mock method so that the sequence of calls can be verified with testing::InSequence. Without the changes in ChunkDemuxer, Init and ErrorWhileParsingClusterAfterInit tests will fail. https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1681: demuxer_->Initialize(&host_, base::Bind(&OnPipelineStatus_NOTREACHED), true); init_cb should not be called here since the demuxer is shutdown before it is initialized (which is considered completed only after initialization segment is fully parsed). This is verified by NOTREACHED. https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1695: demuxer_->Initialize(&host_, base::Bind(&OnPipelineStatus_NOTREACHED), true); ditto https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1766: InSequence s; Added to verify that init_cb is called before OnDemuxerError. If we add InSequence without making the changes in DemuxerError, this test will fail.
dalecurtis@chromium.org changed reviewers: + wolenetz@chromium.org
lg2m, but =>wolenetz as MSE owner.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:430: const PipelineStatusCB& cb, nit on naming: This code is old. We use |status_cb| in demuxer.h, and |cb| here. Then we use |init_cb_| for the saved callback. Maybe we should just always use |init_cb|. https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1681: demuxer_->Initialize(&host_, base::Bind(&OnPipelineStatus_NOTREACHED), true); On 2016/09/16 22:10:16, alokp wrote: > init_cb should not be called here since the demuxer is shutdown before it is > initialized (which is considered completed only after initialization segment is > fully parsed). This is verified by NOTREACHED. hmm, we typically don't like outstanding callbacks and require all callbacks to be fired. So if the demuxer is destroyed before it finishes demuxer init, it should fire the callback with an error status, rather than just hold the callback.
https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:430: const PipelineStatusCB& cb, On 2016/09/16 23:41:47, xhwang (slow) wrote: > nit on naming: This code is old. We use |status_cb| in demuxer.h, and |cb| here. > Then we use |init_cb_| for the saved callback. Maybe we should just always use > |init_cb|. IIUC do you want me to change the function argument name cb -> init_cb? https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1681: demuxer_->Initialize(&host_, base::Bind(&OnPipelineStatus_NOTREACHED), true); On 2016/09/16 23:41:47, xhwang (slow) wrote: > On 2016/09/16 22:10:16, alokp wrote: > > init_cb should not be called here since the demuxer is shutdown before it is > > initialized (which is considered completed only after initialization segment > is > > fully parsed). This is verified by NOTREACHED. > > hmm, we typically don't like outstanding callbacks and require all callbacks to > be fired. So if the demuxer is destroyed before it finishes demuxer init, it > should fire the callback with an error status, rather than just hold the > callback. This patch does not change this behavior. ChunkDemuxer never fired the init_cb if destroyed before init completes. I had to change the completion callback here because CreateInitDoneCB now expects InitDoneCB to be called, which ChunkDemuxer does not. So I made it explicit. If we want to change this behavior, it should be done in a separate patch.
https://chromiumcodereview.appspot.com/2273273002/diff/20001/media/filters/ch... File media/filters/chunk_demuxer.cc (right): https://chromiumcodereview.appspot.com/2273273002/diff/20001/media/filters/ch... media/filters/chunk_demuxer.cc:430: const PipelineStatusCB& cb, On 2016/09/17 00:06:05, alokp wrote: > On 2016/09/16 23:41:47, xhwang (slow) wrote: > > nit on naming: This code is old. We use |status_cb| in demuxer.h, and |cb| > here. > > Then we use |init_cb_| for the saved callback. Maybe we should just always use > > |init_cb|. > > IIUC do you want me to change the function argument name cb -> init_cb? A TODO for the renaming should be good. Thank you! https://chromiumcodereview.appspot.com/2273273002/diff/20001/media/filters/ch... File media/filters/chunk_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/2273273002/diff/20001/media/filters/ch... media/filters/chunk_demuxer_unittest.cc:1681: demuxer_->Initialize(&host_, base::Bind(&OnPipelineStatus_NOTREACHED), true); On 2016/09/17 00:06:05, alokp wrote: > On 2016/09/16 23:41:47, xhwang (slow) wrote: > > On 2016/09/16 22:10:16, alokp wrote: > > > init_cb should not be called here since the demuxer is shutdown before it is > > > initialized (which is considered completed only after initialization segment > > is > > > fully parsed). This is verified by NOTREACHED. > > > > hmm, we typically don't like outstanding callbacks and require all callbacks > to > > be fired. So if the demuxer is destroyed before it finishes demuxer init, it > > should fire the callback with an error status, rather than just hold the > > callback. > > This patch does not change this behavior. ChunkDemuxer never fired the init_cb > if destroyed before init completes. I had to change the completion callback here > because CreateInitDoneCB now expects InitDoneCB to be called, which ChunkDemuxer > does not. So I made it explicit. If we want to change this behavior, it should > be done in a separate patch. The demuxer API is a bit old and it's not very clear about whether the callback should always be fired. I agree such changes (if any) don't belong to this CL. But then I guess for a demuxer implementation, it's legit to either fire the callback, or not fire the callback, when it's destructed before init is finished. With that, I wonder what's the value of asserting that the callback will not be called in these tests? I kinda feel we are testing implementation detail, not the API. Can we just use Bind(&DemuxerInitialized) here but don't expect it to be called? If we "regress" that the callback is actually fired, we'll get a warning about uninterested call, but will not fail.
https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:430: const PipelineStatusCB& cb, On 2016/09/17 03:58:24, xhwang (slow) wrote: > On 2016/09/17 00:06:05, alokp wrote: > > On 2016/09/16 23:41:47, xhwang (slow) wrote: > > > nit on naming: This code is old. We use |status_cb| in demuxer.h, and |cb| > > here. > > > Then we use |init_cb_| for the saved callback. Maybe we should just always > use > > > |init_cb|. > > > > IIUC do you want me to change the function argument name cb -> init_cb? > > A TODO for the renaming should be good. Thank you! I can make this trivial change while I am already here. I just wanted to confirm if this what you wanted me to do. https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/20001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1681: demuxer_->Initialize(&host_, base::Bind(&OnPipelineStatus_NOTREACHED), true); On 2016/09/17 03:58:24, xhwang (slow) wrote: > On 2016/09/17 00:06:05, alokp wrote: > > On 2016/09/16 23:41:47, xhwang (slow) wrote: > > > On 2016/09/16 22:10:16, alokp wrote: > > > > init_cb should not be called here since the demuxer is shutdown before it > is > > > > initialized (which is considered completed only after initialization > segment > > > is > > > > fully parsed). This is verified by NOTREACHED. > > > > > > hmm, we typically don't like outstanding callbacks and require all callbacks > > to > > > be fired. So if the demuxer is destroyed before it finishes demuxer init, it > > > should fire the callback with an error status, rather than just hold the > > > callback. > > > > This patch does not change this behavior. ChunkDemuxer never fired the init_cb > > if destroyed before init completes. I had to change the completion callback > here > > because CreateInitDoneCB now expects InitDoneCB to be called, which > ChunkDemuxer > > does not. So I made it explicit. If we want to change this behavior, it should > > be done in a separate patch. > > The demuxer API is a bit old and it's not very clear about whether the callback > should always be fired. I agree such changes (if any) don't belong to this CL. > But then I guess for a demuxer implementation, it's legit to either fire the > callback, or not fire the callback, when it's destructed before init is > finished. With that, I wonder what's the value of asserting that the callback > will not be called in these tests? I kinda feel we are testing implementation > detail, not the API. > > Can we just use Bind(&DemuxerInitialized) here but don't expect it to be called? > If we "regress" that the callback is actually fired, we'll get a warning about > uninterested call, but will not fail. I see. I misunderstood your previous comment. I agree about not over-testing. Done.
LGTM with one last comment https://codereview.chromium.org/2273273002/diff/40001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/40001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1678: base::Bind(&ChunkDemuxerTest_Shutdown_BeforeAllInitSegmentsAppended_Test:: Will &ChunkDemuxerTest:: work?
https://codereview.chromium.org/2273273002/diff/40001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/40001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1678: base::Bind(&ChunkDemuxerTest_Shutdown_BeforeAllInitSegmentsAppended_Test:: On 2016/09/19 17:35:12, xhwang (slow) wrote: > Will &ChunkDemuxerTest:: work? I wish. Sorry for the ugliness. The only other way is to move the mock callback functions into a mock class, but that seemed like a bigger change.
https://codereview.chromium.org/2273273002/diff/40001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/40001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1678: base::Bind(&ChunkDemuxerTest_Shutdown_BeforeAllInitSegmentsAppended_Test:: On 2016/09/19 17:43:20, alokp wrote: > On 2016/09/19 17:35:12, xhwang (slow) wrote: > > Will &ChunkDemuxerTest:: work? > > I wish. Sorry for the ugliness. > > The only other way is to move the mock callback functions into a mock class, but > that seemed like a bigger change. Made the mock method public as discussed offline.
wolenetz@: ping :)
Sorry - was OoO sick Mon-Tue and am catching up still today. I'm wondering a bit more about what is currently really broken without this CL: https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:435: // Init cb must only be run after this method returns, so always post. nit: s/always// https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:1037: host_->OnDemuxerError(error); Doesn't the host trampoline this back to the media thread? ReportError_Locked is run on the render main thread from what I can tell. So is OnSourceInitDone. So, IIUC, without this CL, init_cb_ gets bound to media thread and posted there from this class in all of its usages (even in ::Initialize()). And in error case shortly after init_cb_ is posted, OnDemuxerError trampolines to media thread, so the ordering should be maintained. Other than unit tests, which might insert their own OnDemuxerError which *doesn't* trampoline, where does this currently fail?
Ah -- I see in the crbug, especially https://bugs.chromium.org/p/chromium/issues/detail?id=633016#c5, the reason (serial runner serially posting, so the thing it posts *after* the init_cb is handled races the posted demuxer error on the media thread). I'll continue this CR in the morning. Sorry for churn/delay on this.
LGTM % fixing the tests to verify expected DemuxerInitialized error or lack thereof in specific cases noted in comments. https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:1037: host_->OnDemuxerError(error); On 2016/09/23 00:11:46, wolenetz wrote: > Doesn't the host trampoline this back to the media thread? ReportError_Locked is > run on the render main thread from what I can tell. So is OnSourceInitDone. > > So, IIUC, without this CL, init_cb_ gets bound to media thread and posted there > from this class in all of its usages (even in ::Initialize()). And in error case > shortly after init_cb_ is posted, OnDemuxerError trampolines to media thread, so > the ordering should be maintained. Other than unit tests, which might insert > their own OnDemuxerError which *doesn't* trampoline, where does this currently > fail? Ignore (see my previous reply on the CL message thread). https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:248: // Public method becuase test cases use it directly. nit: spelling https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1674: // Make sure that the demuxer reports an error if Shutdown() Where is the error verified now? (The err is because ShutDown() occurred before *both* audio and video init segments appended to the distinct sourceIDs setup at the beginning of the test.) https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1694: demuxer_->Initialize(&host_, base::Bind(&ChunkDemuxerTest::DemuxerInitialized, Ditto. https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3223: demuxer_->Initialize(&host_, base::Bind(&ChunkDemuxerTest::DemuxerInitialized, Where is *lack* of error on CDT::DemuxerInitialized now verified?
https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:435: // Init cb must only be run after this method returns, so always post. On 2016/09/23 00:11:46, wolenetz wrote: > nit: s/always// Done. https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:1037: host_->OnDemuxerError(error); On 2016/09/27 21:01:14, wolenetz wrote: > On 2016/09/23 00:11:46, wolenetz wrote: > > Doesn't the host trampoline this back to the media thread? ReportError_Locked > is > > run on the render main thread from what I can tell. So is OnSourceInitDone. > > > > So, IIUC, without this CL, init_cb_ gets bound to media thread and posted > there > > from this class in all of its usages (even in ::Initialize()). And in error > case > > shortly after init_cb_ is posted, OnDemuxerError trampolines to media thread, > so > > the ordering should be maintained. Other than unit tests, which might insert > > their own OnDemuxerError which *doesn't* trampoline, where does this currently > > fail? > > Ignore (see my previous reply on the CL message thread). Acknowledged. https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:248: // Public method becuase test cases use it directly. On 2016/09/27 21:01:14, wolenetz wrote: > nit: spelling Done. https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1674: // Make sure that the demuxer reports an error if Shutdown() On 2016/09/27 21:01:14, wolenetz wrote: > Where is the error verified now? > (The err is because ShutDown() occurred before *both* audio and video init > segments appended to the distinct sourceIDs setup at the beginning of the test.) The error was never being verified (silently) because the init_cb does not get called if Shutdown occurs before Demuxer is initialized. ChunkDemuxer::Shutdown or the destructor, does not do anything with init_cb. Do you want to handle it? I would be happy to make that change but I think it should be done in a separate patch. https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1694: demuxer_->Initialize(&host_, base::Bind(&ChunkDemuxerTest::DemuxerInitialized, On 2016/09/27 21:01:14, wolenetz wrote: > Ditto. ditto. https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3223: demuxer_->Initialize(&host_, base::Bind(&ChunkDemuxerTest::DemuxerInitialized, On 2016/09/27 21:01:14, wolenetz wrote: > Where is *lack* of error on CDT::DemuxerInitialized now verified? This again was silently not getting verified because the test completes before Demuxer initializes. I found about this after I added EXPECT_CALL in CreateInitDoneCB, which was not getting satisfied.
https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1674: // Make sure that the demuxer reports an error if Shutdown() On 2016/09/28 17:22:45, alokp wrote: > On 2016/09/27 21:01:14, wolenetz wrote: > > Where is the error verified now? > > (The err is because ShutDown() occurred before *both* audio and video init > > segments appended to the distinct sourceIDs setup at the beginning of the > test.) > > The error was never being verified (silently) because the init_cb does not get > called if Shutdown occurs before Demuxer is initialized. ChunkDemuxer::Shutdown > or the destructor, does not do anything with init_cb. Do you want to handle it? > I would be happy to make that change but I think it should be done in a separate > patch. Hmm. If there is a demuxer error that isn't reported to UMA because demuxer was shutdown before the init completed, that sounds like an error we'd really like to count as such. If the error does *not* get reported due to the SHutdown here, then please fix the comment, add a crbug and TODO to get such reporting fixed and verified later. Thanks for digging into this! https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1694: demuxer_->Initialize(&host_, base::Bind(&ChunkDemuxerTest::DemuxerInitialized, On 2016/09/28 17:22:45, alokp wrote: > On 2016/09/27 21:01:14, wolenetz wrote: > > Ditto. > > ditto. ditto :) https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3223: demuxer_->Initialize(&host_, base::Bind(&ChunkDemuxerTest::DemuxerInitialized, On 2016/09/28 17:22:45, alokp wrote: > On 2016/09/27 21:01:14, wolenetz wrote: > > Where is *lack* of error on CDT::DemuxerInitialized now verified? > > This again was silently not getting verified because the test completes before > Demuxer initializes. I found about this after I added EXPECT_CALL in > CreateInitDoneCB, which was not getting satisfied. I wonder if this could be fixed with a test class destructor calling RunUntilIdle() on the media thread. I would like positive verification of expected initialization results, or at least lack of any error being dropped too early by early test completion.
https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1674: // Make sure that the demuxer reports an error if Shutdown() On 2016/10/03 21:16:23, wolenetz wrote: > On 2016/09/28 17:22:45, alokp wrote: > > On 2016/09/27 21:01:14, wolenetz wrote: > > > Where is the error verified now? > > > (The err is because ShutDown() occurred before *both* audio and video init > > > segments appended to the distinct sourceIDs setup at the beginning of the > > test.) > > > > The error was never being verified (silently) because the init_cb does not get > > called if Shutdown occurs before Demuxer is initialized. > ChunkDemuxer::Shutdown > > or the destructor, does not do anything with init_cb. Do you want to handle > it? > > I would be happy to make that change but I think it should be done in a > separate > > patch. > > Hmm. If there is a demuxer error that isn't reported to UMA because demuxer was > shutdown before the init completed, that sounds like an error we'd really like > to count as such. If the error does *not* get reported due to the SHutdown here, > then please fix the comment, add a crbug and TODO to get such reporting fixed > and verified later. Thanks for digging into this! Is shutting down before appending initialization segment an error? AFAICT this test does not append any invalid segment, so it should not trigger any initialization error. It just shuts down the demuxer before appending the video segment. https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3223: demuxer_->Initialize(&host_, base::Bind(&ChunkDemuxerTest::DemuxerInitialized, On 2016/10/03 21:16:23, wolenetz wrote: > On 2016/09/28 17:22:45, alokp wrote: > > On 2016/09/27 21:01:14, wolenetz wrote: > > > Where is *lack* of error on CDT::DemuxerInitialized now verified? > > > > This again was silently not getting verified because the test completes before > > Demuxer initializes. I found about this after I added EXPECT_CALL in > > CreateInitDoneCB, which was not getting satisfied. > > I wonder if this could be fixed with a test class destructor calling > RunUntilIdle() on the media thread. I would like positive verification of > expected initialization results, or at least lack of any error being dropped too > early by early test completion. RunUntilIdle does not help here because init_cb is not even posted. The test exits while demuxer was waiting for initialization segments to be appended. out/Debug/media_unittests --gtest_filter=ChunkDemuxerTest.GetBufferedRangesBeforeInitSegment --vmodule=*/media/*=4 [ RUN ] ChunkDemuxerTest.GetBufferedRangesBeforeInitSegment [66548:66548:1004/134303:7701287375808:VERBOSE1:chunk_demuxer.cc(430)] Init() [66548:66548:1004/134303:7701287375868:VERBOSE1:chunk_demuxer.cc(1045)] ChunkDemuxer::ChangeState_Locked() : 0 -> 1 [66548:66548:1004/134303:7701287375948:VERBOSE1:chunk_demuxer.cc(582)] AddId id=audio mime_type=audio/webm codecs=vorbis [66548:66548:1004/134303:7701287376374:VERBOSE2:frame_processor.cc(169)] FrameProcessor() [66548:66548:1004/134303:7701287376450:VERBOSE1:webm_stream_parser.cc(121)] ChangeState() : 0 -> 1 [66548:66548:1004/134303:7701287376502:VERBOSE1:chunk_demuxer.cc(582)] AddId id=video mime_type=video/webm codecs=vp8 [66548:66548:1004/134303:7701287376567:VERBOSE2:frame_processor.cc(169)] FrameProcessor() [66548:66548:1004/134303:7701287376632:VERBOSE1:webm_stream_parser.cc(121)] ChangeState() : 0 -> 1 [66548:66548:1004/134303:7701287376733:VERBOSE1:chunk_demuxer.cc(1021)] Shutdown() [66548:66548:1004/134303:7701287376768:VERBOSE1:chunk_demuxer.cc(1045)] ChunkDemuxer::ChangeState_Locked() : 1 -> 5 [66548:66548:1004/134303:7701287376804:VERBOSE2:frame_processor.cc(174)] ~FrameProcessor() [66548:66548:1004/134303:7701287376852:VERBOSE2:frame_processor.cc(174)] ~FrameProcessor() ../../media/filters/chunk_demuxer_unittest.cc:699: Failure Actual function call count doesn't match EXPECT_CALL(*this, DemuxerInitialized(expected_status))... Expected: to be called once Actual: never called - unsatisfied and active [ FAILED ] ChunkDemuxerTest.GetBufferedRangesBeforeInitSegment (2 ms) [----------] 1 test from ChunkDemuxerTest (2 ms total)
Apologies for review churn. lgtm2 https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:801: EXPECT_CALL(*this, DemuxerInitialized(expected_status)); Good :) https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1674: // Make sure that the demuxer reports an error if Shutdown() On 2016/10/04 20:48:40, alokp wrote: > On 2016/10/03 21:16:23, wolenetz wrote: > > On 2016/09/28 17:22:45, alokp wrote: > > > On 2016/09/27 21:01:14, wolenetz wrote: > > > > Where is the error verified now? > > > > (The err is because ShutDown() occurred before *both* audio and video init > > > > segments appended to the distinct sourceIDs setup at the beginning of the > > > test.) > > > > > > The error was never being verified (silently) because the init_cb does not > get > > > called if Shutdown occurs before Demuxer is initialized. > > ChunkDemuxer::Shutdown > > > or the destructor, does not do anything with init_cb. Do you want to handle > > it? > > > I would be happy to make that change but I think it should be done in a > > separate > > > patch. > > > > Hmm. If there is a demuxer error that isn't reported to UMA because demuxer > was > > shutdown before the init completed, that sounds like an error we'd really like > > to count as such. If the error does *not* get reported due to the SHutdown > here, > > then please fix the comment, add a crbug and TODO to get such reporting fixed > > and verified later. Thanks for digging into this! > > Is shutting down before appending initialization segment an error? AFAICT this > test does not append any invalid segment, so it should not trigger any > initialization error. It just shuts down the demuxer before appending the video > segment. Ok. Thanks for clarifying. This test's comment is "make sure the demuxer reports an error". But you're totally right: it's not an error to shutdown the demuxer before any/all initialization segments have been appended. So I think the test comment should at least be removed. https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1694: demuxer_->Initialize(&host_, base::Bind(&ChunkDemuxerTest::DemuxerInitialized, On 2016/10/03 21:16:23, wolenetz wrote: > On 2016/09/28 17:22:45, alokp wrote: > > On 2016/09/27 21:01:14, wolenetz wrote: > > > Ditto. > > > > ditto. > > ditto :) ignore https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3223: demuxer_->Initialize(&host_, base::Bind(&ChunkDemuxerTest::DemuxerInitialized, On 2016/10/04 20:48:40, alokp wrote: > On 2016/10/03 21:16:23, wolenetz wrote: > > On 2016/09/28 17:22:45, alokp wrote: > > > On 2016/09/27 21:01:14, wolenetz wrote: > > > > Where is *lack* of error on CDT::DemuxerInitialized now verified? > > > > > > This again was silently not getting verified because the test completes > before > > > Demuxer initializes. I found about this after I added EXPECT_CALL in > > > CreateInitDoneCB, which was not getting satisfied. > > > > I wonder if this could be fixed with a test class destructor calling > > RunUntilIdle() on the media thread. I would like positive verification of > > expected initialization results, or at least lack of any error being dropped > too > > early by early test completion. > > RunUntilIdle does not help here because init_cb is not even posted. The test > exits while demuxer was waiting for initialization segments to be appended. > > out/Debug/media_unittests > --gtest_filter=ChunkDemuxerTest.GetBufferedRangesBeforeInitSegment > --vmodule=*/media/*=4 > > [ RUN ] ChunkDemuxerTest.GetBufferedRangesBeforeInitSegment > [66548:66548:1004/134303:7701287375808:VERBOSE1:chunk_demuxer.cc(430)] Init() > [66548:66548:1004/134303:7701287375868:VERBOSE1:chunk_demuxer.cc(1045)] > ChunkDemuxer::ChangeState_Locked() : 0 -> 1 > [66548:66548:1004/134303:7701287375948:VERBOSE1:chunk_demuxer.cc(582)] AddId > id=audio mime_type=audio/webm codecs=vorbis > [66548:66548:1004/134303:7701287376374:VERBOSE2:frame_processor.cc(169)] > FrameProcessor() > [66548:66548:1004/134303:7701287376450:VERBOSE1:webm_stream_parser.cc(121)] > ChangeState() : 0 -> 1 > [66548:66548:1004/134303:7701287376502:VERBOSE1:chunk_demuxer.cc(582)] AddId > id=video mime_type=video/webm codecs=vp8 > [66548:66548:1004/134303:7701287376567:VERBOSE2:frame_processor.cc(169)] > FrameProcessor() > [66548:66548:1004/134303:7701287376632:VERBOSE1:webm_stream_parser.cc(121)] > ChangeState() : 0 -> 1 > [66548:66548:1004/134303:7701287376733:VERBOSE1:chunk_demuxer.cc(1021)] > Shutdown() > [66548:66548:1004/134303:7701287376768:VERBOSE1:chunk_demuxer.cc(1045)] > ChunkDemuxer::ChangeState_Locked() : 1 -> 5 > [66548:66548:1004/134303:7701287376804:VERBOSE2:frame_processor.cc(174)] > ~FrameProcessor() > [66548:66548:1004/134303:7701287376852:VERBOSE2:frame_processor.cc(174)] > ~FrameProcessor() > ../../media/filters/chunk_demuxer_unittest.cc:699: Failure > Actual function call count doesn't match EXPECT_CALL(*this, > DemuxerInitialized(expected_status))... > Expected: to be called once > Actual: never called - unsatisfied and active > [ FAILED ] ChunkDemuxerTest.GetBufferedRangesBeforeInitSegment (2 ms) > [----------] 1 test from ChunkDemuxerTest (2 ms total) I see. My comment was that RunUntilIdle() might let unexpected posted error cb to fire and get caught by strictmock, but that's reasonably out of scope of this CL. (I wasn't asking for EXPECT_CALL(init_cb_ called with some status) to be added back to this test, since that cb shouldn't get called until after any/all expected (1 per each AddId) initial init segments are successfully appended (or some error), unlike what I think the most recent previous reply on this comment was doing.)
On 2016/10/05 23:41:24, wolenetz wrote: > Apologies for review churn. lgtm2 Correction: lgtm2 % nit: "So I think the test comment should at least be removed."
https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1674: // Make sure that the demuxer reports an error if Shutdown() On 2016/10/05 23:41:23, wolenetz wrote: > On 2016/10/04 20:48:40, alokp wrote: > > On 2016/10/03 21:16:23, wolenetz wrote: > > > On 2016/09/28 17:22:45, alokp wrote: > > > > On 2016/09/27 21:01:14, wolenetz wrote: > > > > > Where is the error verified now? > > > > > (The err is because ShutDown() occurred before *both* audio and video > init > > > > > segments appended to the distinct sourceIDs setup at the beginning of > the > > > > test.) > > > > > > > > The error was never being verified (silently) because the init_cb does not > > get > > > > called if Shutdown occurs before Demuxer is initialized. > > > ChunkDemuxer::Shutdown > > > > or the destructor, does not do anything with init_cb. Do you want to > handle > > > it? > > > > I would be happy to make that change but I think it should be done in a > > > separate > > > > patch. > > > > > > Hmm. If there is a demuxer error that isn't reported to UMA because demuxer > > was > > > shutdown before the init completed, that sounds like an error we'd really > like > > > to count as such. If the error does *not* get reported due to the SHutdown > > here, > > > then please fix the comment, add a crbug and TODO to get such reporting > fixed > > > and verified later. Thanks for digging into this! > > > > Is shutting down before appending initialization segment an error? AFAICT this > > test does not append any invalid segment, so it should not trigger any > > initialization error. It just shuts down the demuxer before appending the > video > > segment. > > Ok. Thanks for clarifying. > > This test's comment is "make sure the demuxer reports an error". But you're > totally right: it's not an error to shutdown the demuxer before any/all > initialization segments have been appended. So I think the test comment should > at least be removed. Done. https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1694: demuxer_->Initialize(&host_, base::Bind(&ChunkDemuxerTest::DemuxerInitialized, On 2016/10/05 23:41:23, wolenetz wrote: > On 2016/10/03 21:16:23, wolenetz wrote: > > On 2016/09/28 17:22:45, alokp wrote: > > > On 2016/09/27 21:01:14, wolenetz wrote: > > > > Ditto. > > > > > > ditto. > > > > ditto :) > > ignore Acknowledged. https://codereview.chromium.org/2273273002/diff/60001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3223: demuxer_->Initialize(&host_, base::Bind(&ChunkDemuxerTest::DemuxerInitialized, On 2016/10/05 23:41:24, wolenetz wrote: > On 2016/10/04 20:48:40, alokp wrote: > > On 2016/10/03 21:16:23, wolenetz wrote: > > > On 2016/09/28 17:22:45, alokp wrote: > > > > On 2016/09/27 21:01:14, wolenetz wrote: > > > > > Where is *lack* of error on CDT::DemuxerInitialized now verified? > > > > > > > > This again was silently not getting verified because the test completes > > before > > > > Demuxer initializes. I found about this after I added EXPECT_CALL in > > > > CreateInitDoneCB, which was not getting satisfied. > > > > > > I wonder if this could be fixed with a test class destructor calling > > > RunUntilIdle() on the media thread. I would like positive verification of > > > expected initialization results, or at least lack of any error being dropped > > too > > > early by early test completion. > > > > RunUntilIdle does not help here because init_cb is not even posted. The test > > exits while demuxer was waiting for initialization segments to be appended. > > > > out/Debug/media_unittests > > --gtest_filter=ChunkDemuxerTest.GetBufferedRangesBeforeInitSegment > > --vmodule=*/media/*=4 > > > > [ RUN ] ChunkDemuxerTest.GetBufferedRangesBeforeInitSegment > > [66548:66548:1004/134303:7701287375808:VERBOSE1:chunk_demuxer.cc(430)] Init() > > [66548:66548:1004/134303:7701287375868:VERBOSE1:chunk_demuxer.cc(1045)] > > ChunkDemuxer::ChangeState_Locked() : 0 -> 1 > > [66548:66548:1004/134303:7701287375948:VERBOSE1:chunk_demuxer.cc(582)] AddId > > id=audio mime_type=audio/webm codecs=vorbis > > [66548:66548:1004/134303:7701287376374:VERBOSE2:frame_processor.cc(169)] > > FrameProcessor() > > [66548:66548:1004/134303:7701287376450:VERBOSE1:webm_stream_parser.cc(121)] > > ChangeState() : 0 -> 1 > > [66548:66548:1004/134303:7701287376502:VERBOSE1:chunk_demuxer.cc(582)] AddId > > id=video mime_type=video/webm codecs=vp8 > > [66548:66548:1004/134303:7701287376567:VERBOSE2:frame_processor.cc(169)] > > FrameProcessor() > > [66548:66548:1004/134303:7701287376632:VERBOSE1:webm_stream_parser.cc(121)] > > ChangeState() : 0 -> 1 > > [66548:66548:1004/134303:7701287376733:VERBOSE1:chunk_demuxer.cc(1021)] > > Shutdown() > > [66548:66548:1004/134303:7701287376768:VERBOSE1:chunk_demuxer.cc(1045)] > > ChunkDemuxer::ChangeState_Locked() : 1 -> 5 > > [66548:66548:1004/134303:7701287376804:VERBOSE2:frame_processor.cc(174)] > > ~FrameProcessor() > > [66548:66548:1004/134303:7701287376852:VERBOSE2:frame_processor.cc(174)] > > ~FrameProcessor() > > ../../media/filters/chunk_demuxer_unittest.cc:699: Failure > > Actual function call count doesn't match EXPECT_CALL(*this, > > DemuxerInitialized(expected_status))... > > Expected: to be called once > > Actual: never called - unsatisfied and active > > [ FAILED ] ChunkDemuxerTest.GetBufferedRangesBeforeInitSegment (2 ms) > > [----------] 1 test from ChunkDemuxerTest (2 ms total) > > I see. My comment was that RunUntilIdle() might let unexpected posted error cb > to fire and get caught by strictmock, but that's reasonably out of scope of this > CL. (I wasn't asking for EXPECT_CALL(init_cb_ called with some status) to be > added back to this test, since that cb shouldn't get called until after any/all > expected (1 per each AddId) initial init segments are successfully appended (or > some error), unlike what I think the most recent previous reply on this comment > was doing.) Acknowledged.
The CQ bit was checked by alokp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/2273273002/#ps160001 (title: "removes comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Directly call ChunkDemuxer::Initialize completion callback. ChunkDemuxer posts init_cb while calls DemuxerHost::OnDemuxerError directly, which creates a race between the two. In some cases if there is an error right after a successful intialization, the error will be reported before init_cb has a chance to run which violates upstream expectations. BUG=633016 ========== to ========== Directly call ChunkDemuxer::Initialize completion callback. ChunkDemuxer posts init_cb while calls DemuxerHost::OnDemuxerError directly, which creates a race between the two. In some cases if there is an error right after a successful intialization, the error will be reported before init_cb has a chance to run which violates upstream expectations. BUG=633016 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Directly call ChunkDemuxer::Initialize completion callback. ChunkDemuxer posts init_cb while calls DemuxerHost::OnDemuxerError directly, which creates a race between the two. In some cases if there is an error right after a successful intialization, the error will be reported before init_cb has a chance to run which violates upstream expectations. BUG=633016 ========== to ========== Directly call ChunkDemuxer::Initialize completion callback. ChunkDemuxer posts init_cb while calls DemuxerHost::OnDemuxerError directly, which creates a race between the two. In some cases if there is an error right after a successful intialization, the error will be reported before init_cb has a chance to run which violates upstream expectations. BUG=633016 Committed: https://crrev.com/0c4e92a05559f716367fdef9928ae6df0110ff65 Cr-Commit-Position: refs/heads/master@{#423463} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/0c4e92a05559f716367fdef9928ae6df0110ff65 Cr-Commit-Position: refs/heads/master@{#423463} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
