Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
257323003:
Remove the IPC to request DemuxerConfigs when config changes (Closed)
|
Created:
6 years, 7 months ago by qinmin Modified:
6 years, 7 months ago CC:
chromium-reviews, fischman+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemove the IPC to request DemuxerConfigs when config changes
When config changes, we currently use a round trip IPC to request the new config.
This increases the overall latency when processing config changes.
This CL appends the new config data to the DemuxerData which contains the config change access unit.
BUG=325528
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269614
Patch Set 1 #
Total comments: 34
Patch Set 2 : addressing comments #
Total comments: 16
Patch Set 3 : addressing comments #
Total comments: 2
Patch Set 4 : remove CopyDemuxerConfig() #
Total comments: 4
Patch Set 5 : nits #
Total comments: 5
Patch Set 6 : addressing comments from jschuh #
Total comments: 8
Patch Set 7 : fixing nits #Patch Set 8 : CHECK the return value of GetDemuxerConfigFromStream() when config changes #Patch Set 9 : adding CHECK to enforce the optional demuxer_configs only has 0 or 1 element #Messages
Total messages: 29 (0 generated)
PTAL
Thank you for splitting this out of https://codereview.chromium.org/254473010/. https://codereview.chromium.org/257323003/diff/1/content/renderer/media/andro... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/257323003/diff/1/content/renderer/media/andro... content/renderer/media/android/media_source_delegate.cc:394: GetDemuxerConfigFromStream(&data->demuxer_configs[0], is_audio); Previous code would null-reference crash if the relative {audio,video}_stream_ were NULL. However, the new GetDemuxerConfigFromStream() conditions filling in the config with stream's config only if the stream pointer is also not-NULL. Here (but not in NotifyDemuxerReady() path) I believe we should CHECK that the associated stream is not NULL. https://codereview.chromium.org/257323003/diff/1/content/renderer/media/andro... File content/renderer/media/android/media_source_delegate.h (right): https://codereview.chromium.org/257323003/diff/1/content/renderer/media/andro... content/renderer/media/android/media_source_delegate.h:101: void OnMediaConfigRequest(); I believe this method is now made obsolete. Remove if so, or correct me if I'm wrong please :) https://codereview.chromium.org/257323003/diff/1/media/base/android/demuxer_a... File media/base/android/demuxer_android.h (right): https://codereview.chromium.org/257323003/diff/1/media/base/android/demuxer_a... media/base/android/demuxer_android.h:44: // Called in response to RequestDemuxerConfigs() and also when the demuxer has Comment references the deleted method. https://codereview.chromium.org/257323003/diff/1/media/base/android/demuxer_a... media/base/android/demuxer_android.h:48: // RequestDemuxerConfigs() to initialize themselves instead of the demuxer ditto https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:105: bool MediaDecoderJob::Decode( It seems to me more understandable to just return a pointer to the configs (instead of true) or NULL (instead of false), thereby eliminating need for caller of Decode to poke us for the configs for these same conditions. We could possibly pass a scoped_ptr back containing a copy of the DemuxerConfigs (or NULL) and thereby retain the ClearData() (see my next comment). https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:125: decode_cb_.Reset(); I'm confused: previously we needed to ClearData(). Now, when does this occur once we've reached this code path? https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:161: int index = NoAccessUnitsRemainingInChunk(true) ? nit: seems like some code duplication of CurrentAccessUnit() here. de-dupe with some "current received data chunk" helper? https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:282: // If the first access unit is a config change, request the player to dequeue nit: Now, if the first AU is a config change, shouldn't the configs already be available in the "current received data chunk"? It seems to me (possibly not for this CL) that this is a bit of a circuitous route. wdyt? https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:510: last_access_unit.status != DemuxerStream::kConfigChanged && If it is kConfigChanged, and ClearData() never occurred, does this job become unable to process any further data? https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:1600: EXPECT_EQ(4, demuxer_->num_data_requests()); // No more data requested yet. nit: remove the incorrect(?) comment https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:1602: // No job re-creation should occur until the requested configs arrive. nit: adjust the wording of comment: Both jobs should have been reconfigured by now. https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:1603: EXPECT_NE(first_audio_job, GetMediaDecoderJob(true)); nit: these pointer checks can be flaky. I think we already have a bug for this -- reference it here, too? https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:1869: TEST_F(MediaSourcePlayerTest, ConfigChangedThenReleaseThenConfigsAvailable) { Test needs renaming (no separation of configchange + later configs available)? Hmm. Also, should we enforce that once we get a previous configsavailable (from notifydemuxerready), that we never again get such a message, since any further config change from the demuxer would be within the demuxerdata chunk? https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:1872: // Test if Release() occurs after |kConfigChanged| detected, new configs nit: comment needs rewording (no configs requested...) https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:1890: TEST_F(MediaSourcePlayerTest, ConfigChangedThenReleaseThenStart) { This test is now entirely a subset of the previous test. Remove this one?
https://codereview.chromium.org/257323003/diff/1/content/renderer/media/andro... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/257323003/diff/1/content/renderer/media/andro... content/renderer/media/android/media_source_delegate.cc:394: GetDemuxerConfigFromStream(&data->demuxer_configs[0], is_audio); On 2014/05/02 22:25:30, wolenetz wrote: > Previous code would null-reference crash if the relative {audio,video}_stream_ > were NULL. However, the new GetDemuxerConfigFromStream() conditions filling in > the config with stream's config only if the stream pointer is also not-NULL. > > Here (but not in NotifyDemuxerReady() path) I believe we should CHECK that the > associated stream is not NULL. Done. https://codereview.chromium.org/257323003/diff/1/content/renderer/media/andro... File content/renderer/media/android/media_source_delegate.h (right): https://codereview.chromium.org/257323003/diff/1/content/renderer/media/andro... content/renderer/media/android/media_source_delegate.h:101: void OnMediaConfigRequest(); On 2014/05/02 22:25:30, wolenetz wrote: > I believe this method is now made obsolete. Remove if so, or correct me if I'm > wrong please :) Done. https://codereview.chromium.org/257323003/diff/1/media/base/android/demuxer_a... File media/base/android/demuxer_android.h (right): https://codereview.chromium.org/257323003/diff/1/media/base/android/demuxer_a... media/base/android/demuxer_android.h:44: // Called in response to RequestDemuxerConfigs() and also when the demuxer has On 2014/05/02 22:25:30, wolenetz wrote: > Comment references the deleted method. Done. https://codereview.chromium.org/257323003/diff/1/media/base/android/demuxer_a... media/base/android/demuxer_android.h:48: // RequestDemuxerConfigs() to initialize themselves instead of the demuxer On 2014/05/02 22:25:30, wolenetz wrote: > ditto Done. https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:105: bool MediaDecoderJob::Decode( On 2014/05/02 22:25:30, wolenetz wrote: > It seems to me more understandable to just return a pointer to the configs > (instead of true) or NULL (instead of false), thereby eliminating need for > caller of Decode to poke us for the configs for these same conditions. We could > possibly pass a scoped_ptr back containing a copy of the DemuxerConfigs (or > NULL) and thereby retain the ClearData() (see my next comment). Done. https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:125: decode_cb_.Reset(); If there is a config change, MediaDecoderJob will be recreated， thus there is no need to clear the data here. Additionally, the data is still needed after this call because MSP needs it to get the DemuxerConfig. And with my coming refactoring patch, we need to keep the data for the inactive chunk after receiving a config change, so we don't need to request it again. On 2014/05/02 22:25:30, wolenetz wrote: > I'm confused: previously we needed to ClearData(). Now, when does this occur > once we've reached this code path? https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:161: int index = NoAccessUnitsRemainingInChunk(true) ? On 2014/05/02 22:25:30, wolenetz wrote: > nit: seems like some code duplication of CurrentAccessUnit() here. de-dupe with > some "current received data chunk" helper? Done. https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:282: // If the first access unit is a config change, request the player to dequeue When MSP calls decode(), if there is no data available, MDJ will call requestData(). And the returned AU might be a config change. In this case, MSP already finishes calling decode(), and is expecting OnDecodeCompleted() to be called. That's why we want MSP to call decode on the same AU again. On 2014/05/02 22:25:30, wolenetz wrote: > nit: Now, if the first AU is a config change, shouldn't the configs already be > available in the "current received data chunk"? It seems to me (possibly not for > this CL) that this is a bit of a circuitous route. wdyt? https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:510: last_access_unit.status != DemuxerStream::kConfigChanged && if it is kConfigChanged, MDJ will get recreated and no further data needs to be processed. On 2014/05/02 22:25:30, wolenetz wrote: > If it is kConfigChanged, and ClearData() never occurred, does this job become > unable to process any further data? https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:1600: EXPECT_EQ(4, demuxer_->num_data_requests()); // No more data requested yet. On 2014/05/02 22:25:30, wolenetz wrote: > nit: remove the incorrect(?) comment Done. https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:1602: // No job re-creation should occur until the requested configs arrive. On 2014/05/02 22:25:30, wolenetz wrote: > nit: adjust the wording of comment: Both jobs should have been reconfigured by > now. Done. https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:1603: EXPECT_NE(first_audio_job, GetMediaDecoderJob(true)); On 2014/05/02 22:25:30, wolenetz wrote: > nit: these pointer checks can be flaky. I think we already have a bug for this > -- reference it here, too? Done. https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:1869: TEST_F(MediaSourcePlayerTest, ConfigChangedThenReleaseThenConfigsAvailable) { Merged this test with the next test. Added a DCHECK in MSP to check that HasVideo() and HasAudio() should both be false when OnDemuxerConfigsAvailable（） is called. And also added a Resume() function here so that we don't call Start() twice in test. On 2014/05/02 22:25:30, wolenetz wrote: > Test needs renaming (no separation of configchange + later configs available)? > Hmm. > Also, should we enforce that once we get a previous configsavailable (from > notifydemuxerready), that we never again get such a message, since any further > config change from the demuxer would be within the demuxerdata chunk? https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:1872: // Test if Release() occurs after |kConfigChanged| detected, new configs On 2014/05/02 22:25:30, wolenetz wrote: > nit: comment needs rewording (no configs requested...) Done. https://codereview.chromium.org/257323003/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:1890: TEST_F(MediaSourcePlayerTest, ConfigChangedThenReleaseThenStart) { merged with the above one On 2014/05/02 22:25:30, wolenetz wrote: > This test is now entirely a subset of the previous test. Remove this one?
Looking pretty good. Just a few more nits and comments: https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:105: bool MediaDecoderJob::Decode( On 2014/05/05 20:52:19, qinmin wrote: > On 2014/05/02 22:25:30, wolenetz wrote: > > It seems to me more understandable to just return a pointer to the configs > > (instead of true) or NULL (instead of false), thereby eliminating need for > > caller of Decode to poke us for the configs for these same conditions. We > could > > possibly pass a scoped_ptr back containing a copy of the DemuxerConfigs (or > > NULL) and thereby retain the ClearData() (see my next comment). > > Done. Oops - I swapped my false / true mapping in my original CR comment. Thanks for catching! https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:125: decode_cb_.Reset(); On 2014/05/05 20:52:19, qinmin wrote: > If there is a config change, MediaDecoderJob will be recreated， thus there is no > need to clear the data here. > Additionally, the data is still needed after this call because MSP needs it to > get the DemuxerConfig. And with my coming refactoring patch, we need to keep the > data for the inactive chunk after receiving a config change, so we don't need to > request it again. > > On 2014/05/02 22:25:30, wolenetz wrote: > > I'm confused: previously we needed to ClearData(). Now, when does this occur > > once we've reached this code path? > sgtm thanks https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:282: // If the first access unit is a config change, request the player to dequeue On 2014/05/05 20:52:19, qinmin wrote: > When MSP calls decode(), if there is no data available, MDJ will call > requestData(). And the returned AU might be a config change. > In this case, MSP already finishes calling decode(), and is expecting > OnDecodeCompleted() to be called. That's why we want MSP to call decode on the > same AU again. > > On 2014/05/02 22:25:30, wolenetz wrote: > > nit: Now, if the first AU is a config change, shouldn't the configs already be > > available in the "current received data chunk"? It seems to me (possibly not > for > > this CL) that this is a bit of a circuitous route. wdyt? > sgtm thanks https://codereview.chromium.org/257323003/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:510: last_access_unit.status != DemuxerStream::kConfigChanged && On 2014/05/05 20:52:19, qinmin wrote: > if it is kConfigChanged, MDJ will get recreated and no further data needs to be > processed. > > On 2014/05/02 22:25:30, wolenetz wrote: > > If it is kConfigChanged, and ClearData() never occurred, does this job become > > unable to process any further data? > sgtm thanks https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:127: return &(received_data_[index].demuxer_configs[0]); Before returning, please protect against [0] which could exceed the bounds of an empty demuxer_configs vector; add something like: CHECK_EQ(1, received_data_[index].demuxer_configs.size()); I'm also concerned about the unspoken lifetime of the returned pointer. ISTM that returning a scoped_ptr to a copy of our MDJ-owned configs would be less fragile and less tightly bound. https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_decoder_job.h:58: // Null otherwise. If the return value is NULL, decode was started and nit: s/Null/NULL https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_decoder_job.h:60: // Returns false if a config change is needed. |callback| is ignored nit: s/Returns false if a config change is needed./If a config change is detected,/ Also, maybe add something to the effect of "In the case of requiring further data before commencing decode, NULL will also be returned although the config change has not yet been detected." ? https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:339: EXPECT_TRUE(!player_.IsPlaying()); nit: s/TRUE(!/FALSE(/ ? https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:351: } I think we should verify that player has respective decoder job(s) for the case where new player {audio,video} data request was expected. Is there any case where this would not apply? I think we have already have a test case where MDJ might exist though no more data request was expected, so previous iff doesn't apply here, right? (But we could do a partial check...) https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1574: // the audio decoder job, and should request new demuxer configs. nit: reword the last portion of this comment. "Player should reconfigure the audio decoder job with the supplied configs." https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1578: // Simulate arrival of new configs. nit: remove old comment https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1886: player_.Start(); nit: use Resume(false, false) here, or is it better to keep the expectations rigidly here?
https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:127: return &(received_data_[index].demuxer_configs[0]); Changed this function to return a new pointer and asked the caller to own it. On 2014/05/05 22:06:55, wolenetz wrote: > Before returning, please protect against [0] which could exceed the bounds of an > empty demuxer_configs vector; add something like: > CHECK_EQ(1, received_data_[index].demuxer_configs.size()); > > I'm also concerned about the unspoken lifetime of the returned pointer. ISTM > that returning a scoped_ptr to a copy of our MDJ-owned configs would be less > fragile and less tightly bound. https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_decoder_job.h:58: // Null otherwise. If the return value is NULL, decode was started and On 2014/05/05 22:06:55, wolenetz wrote: > nit: s/Null/NULL Done. https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_decoder_job.h:60: // Returns false if a config change is needed. |callback| is ignored On 2014/05/05 22:06:55, wolenetz wrote: > nit: s/Returns false if a config change is needed./If a config change is > detected,/ > > Also, maybe add something to the effect of "In the case of requiring further > data before commencing decode, NULL will also be returned although the config > change has not yet been detected." ? Done. https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:339: EXPECT_TRUE(!player_.IsPlaying()); On 2014/05/05 22:06:55, wolenetz wrote: > nit: s/TRUE(!/FALSE(/ ? Done. https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:351: } added the check to verify that if expect_player_requests is true, then corresponding GetMediaDecoderJob() will also be true. On 2014/05/05 22:06:55, wolenetz wrote: > I think we should verify that player has respective decoder job(s) for the case > where new player {audio,video} data request was expected. Is there any case > where this would not apply? > > I think we have already have a test case where MDJ might exist though no more > data request was expected, so previous iff doesn't apply here, right? (But we > could do a partial check...) https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1574: // the audio decoder job, and should request new demuxer configs. On 2014/05/05 22:06:55, wolenetz wrote: > nit: reword the last portion of this comment. "Player should reconfigure the > audio decoder job with the supplied configs." Done. https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1578: // Simulate arrival of new configs. On 2014/05/05 22:06:55, wolenetz wrote: > nit: remove old comment Done. https://codereview.chromium.org/257323003/diff/20001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1886: player_.Start(); On 2014/05/05 22:06:55, wolenetz wrote: > nit: use Resume(false, false) here, or is it better to keep the expectations > rigidly here? Done.
https://codereview.chromium.org/257323003/diff/40001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/257323003/diff/40001/media/base/android/media... media/base/android/media_decoder_job.cc:141: CopyDemuxerConfig(&(received_data_[index].demuxer_configs[0]), config); Perhaps my C++ fu is failing me, but: Since DemuxerConfigs is a struct, can't we just do *config = received_data_[index].demuxer_configs[0] here? Or even better just put it in the constructor like DemuxerConfigs* config = new DemuxerConfigs(received_data_[index].demuxer_configs[0])? Or (my preferred option) conform to chromium smart pointer guidelines (http://www.chromium.org/developers/smart-pointer-guidelines) and replace the last three lines of this method with: return scoped_ptr<DemuxerConfigs>(new DemuxerConfigs(received_data_[index].demuxer_configs[0])); (and change the method declaration + definition to return a scoped_ptr<DemuxerConfigs> instead of a DemuxerConfigs*. Each of these options would address my concern that CopyDemuxerConfig(), defined in this file, is fragile to DemuxerConfigs changes, defined in demuxer_stream_player_params.h. If we must keep CopyDemuxerConfig, s/Config/Configs/ please.
https://codereview.chromium.org/257323003/diff/40001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/257323003/diff/40001/media/base/android/media... media/base/android/media_decoder_job.cc:141: CopyDemuxerConfig(&(received_data_[index].demuxer_configs[0]), config); Done. passing a scoped_ptr to the caller now On 2014/05/06 19:06:11, wolenetz wrote: > Perhaps my C++ fu is failing me, but: > > Since DemuxerConfigs is a struct, can't we just do *config = > received_data_[index].demuxer_configs[0] here? > > Or even better just put it in the constructor like DemuxerConfigs* config = new > DemuxerConfigs(received_data_[index].demuxer_configs[0])? > > Or (my preferred option) conform to chromium smart pointer guidelines > (http://www.chromium.org/developers/smart-pointer-guidelines) and replace the > last three lines of this method with: > return scoped_ptr<DemuxerConfigs>(new > DemuxerConfigs(received_data_[index].demuxer_configs[0])); > (and change the method declaration + definition to return a > scoped_ptr<DemuxerConfigs> instead of a DemuxerConfigs*. > > Each of these options would address my concern that CopyDemuxerConfig(), defined > in this file, is fragile to DemuxerConfigs changes, defined in > demuxer_stream_player_params.h. If we must keep CopyDemuxerConfig, > s/Config/Configs/ please.
lgtm % nits. Thank you. https://codereview.chromium.org/257323003/diff/60001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/257323003/diff/60001/media/base/android/media... media/base/android/media_source_player.cc:658: true)).Pass()); nit: I'm not sure .Pass() is needed here. Looking at examples where a utility method returns a temporary scoped_ptr<>, callers that assign that retval to a local do not use Pass(). See use of AudioBus::Create() for one example, here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/me... Is .Pass() necessary when using configs(....) instead of configs = .... ? https://codereview.chromium.org/257323003/diff/60001/media/base/android/media... media/base/android/media_source_player.cc:690: false)).Pass()); nit: ditto
https://codereview.chromium.org/257323003/diff/60001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/257323003/diff/60001/media/base/android/media... media/base/android/media_source_player.cc:658: true)).Pass()); Done. On 2014/05/06 19:58:49, wolenetz wrote: > nit: I'm not sure .Pass() is needed here. > Looking at examples where a utility method returns a temporary scoped_ptr<>, > callers that assign that retval to a local do not use Pass(). See use of > AudioBus::Create() for one example, here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/me... > > Is .Pass() necessary when using configs(....) instead of configs = .... ? https://codereview.chromium.org/257323003/diff/60001/media/base/android/media... media/base/android/media_source_player.cc:690: false)).Pass()); On 2014/05/06 19:58:49, wolenetz wrote: > nit: ditto Done.
+jschuh for content/common/media/media_player_messages_android.h
jschuh@, PTAL content/common/media/media_player_messages_android.h
ping, jschuh@, would you please take a look at the IPC change? thanks
Sorry, I was out all last week and am still catching up. https://codereview.chromium.org/257323003/diff/80001/content/renderer/media/a... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/257323003/diff/80001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.cc:394: data->demuxer_configs.resize(1); It looks like we could end up sending an empty config here if GetDemuxerConfigFromStream fails. Seems like it would be better for GetDemuxerConfigFromStream to return e.g. a boolean indicating success, so we know whether we should send the config (or maybe abort the message entirely). https://codereview.chromium.org/257323003/diff/80001/media/base/android/demux... File media/base/android/demuxer_stream_player_params.h (right): https://codereview.chromium.org/257323003/diff/80001/media/base/android/demux... media/base/android/demuxer_stream_player_params.h:62: std::vector<DemuxerConfigs> demuxer_configs; I'm a bit confused. Are you using the vector to represent an optional field or do you anticipate sending mutliple configs at some point?
https://codereview.chromium.org/257323003/diff/80001/content/renderer/media/a... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/257323003/diff/80001/content/renderer/media/a... content/renderer/media/android/media_source_delegate.cc:394: data->demuxer_configs.resize(1); On 2014/05/08 16:52:53, Justin Schuh wrote: > It looks like we could end up sending an empty config here if > GetDemuxerConfigFromStream fails. Seems like it would be better for > GetDemuxerConfigFromStream to return e.g. a boolean indicating success, so we > know whether we should send the config (or maybe abort the message entirely). Done. https://codereview.chromium.org/257323003/diff/80001/media/base/android/demux... File media/base/android/demuxer_stream_player_params.h (right): https://codereview.chromium.org/257323003/diff/80001/media/base/android/demux... media/base/android/demuxer_stream_player_params.h:62: std::vector<DemuxerConfigs> demuxer_configs; This represents an optional field, there should be at most one DemuxerConfigs in this IPC On 2014/05/08 16:52:53, Justin Schuh wrote: > I'm a bit confused. Are you using the vector to represent an optional field or > do you anticipate sending mutliple configs at some point?
I'm chiming in again. One comment, 2 nits: https://codereview.chromium.org/257323003/diff/100001/content/renderer/media/... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/257323003/diff/100001/content/renderer/media/... content/renderer/media/android/media_source_delegate.cc:350: void MediaSourceDelegate::ReadFromDemuxerStream(media::DemuxerStream::Type type, not lgtm: this mismatches declaration's retval. Why is decl now bool? https://codereview.chromium.org/257323003/diff/100001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/257323003/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.cc:124: // Clear received data because we need to handle a config change. nit: we no longer clear the data. Sorry I didn't catch this nit earlier... https://codereview.chromium.org/257323003/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.cc:134: MEDIA_CODEC_OK, nit: I think this is ok, but we would miss learning about broken demuxer that is incorrectly signalling config change without providing config. Would MEDIA_CODEC_ERROR be better? I liked the previous CHECK because it would explicitly crash on this incorrect demuxer behavior. wdyt?
https://codereview.chromium.org/257323003/diff/100001/content/renderer/media/... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/257323003/diff/100001/content/renderer/media/... content/renderer/media/android/media_source_delegate.cc:350: void MediaSourceDelegate::ReadFromDemuxerStream(media::DemuxerStream::Type type, This should still be a void, accidentally changed it to bool while modifying GetDemuxerConfigFromStream() On 2014/05/08 17:41:47, wolenetz wrote: > not lgtm: this mismatches declaration's retval. Why is decl now bool? https://codereview.chromium.org/257323003/diff/100001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/257323003/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.cc:124: // Clear received data because we need to handle a config change. On 2014/05/08 17:41:47, wolenetz wrote: > nit: we no longer clear the data. Sorry I didn't catch this nit earlier... Done. https://codereview.chromium.org/257323003/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.cc:134: MEDIA_CODEC_OK, The crash will crash the browser process, probably a MEDIA_CODEC_ERROR will be better. On 2014/05/08 17:41:47, wolenetz wrote: > nit: I think this is ok, but we would miss learning about broken demuxer that is > incorrectly signalling config change without providing config. Would > MEDIA_CODEC_ERROR be better? I liked the previous CHECK because it would > explicitly crash on this incorrect demuxer behavior. wdyt?
https://codereview.chromium.org/257323003/diff/80001/media/base/android/demux... File media/base/android/demuxer_stream_player_params.h (right): https://codereview.chromium.org/257323003/diff/80001/media/base/android/demux... media/base/android/demuxer_stream_player_params.h:62: std::vector<DemuxerConfigs> demuxer_configs; On 2014/05/08 16:52:53, Justin Schuh wrote: > I'm a bit confused. Are you using the vector to represent an optional field or > do you anticipate sending mutliple configs at some point? Justin, do you prefer a vector or a separate ParamTraits with defined read/write behavior for an optional field? Not sure what is the general rule for this. Thanks
lgtm % nit https://codereview.chromium.org/257323003/diff/100001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/257323003/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.cc:134: MEDIA_CODEC_OK, On 2014/05/08 17:48:37, qinmin wrote: > The crash will crash the browser process, probably a MEDIA_CODEC_ERROR will be > better. > > On 2014/05/08 17:41:47, wolenetz wrote: > > nit: I think this is ok, but we would miss learning about broken demuxer that > is > > incorrectly signalling config change without providing config. Would > > MEDIA_CODEC_ERROR be better? I liked the previous CHECK because it would > > explicitly crash on this incorrect demuxer behavior. wdyt? > nit: Can we do a CHECK, then, in media_source_delegate, to get crash for this situation emitted there? This crash should not occur, so if it does, it may be low-frequency and MEDIA_CODEC_ERROR to a few users might not get attention directed to getting a fix. wdyt?
https://codereview.chromium.org/257323003/diff/100001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/257323003/diff/100001/media/base/android/medi... media/base/android/media_decoder_job.cc:134: MEDIA_CODEC_OK, Sounds ok to me. modified the media_source_delegate.cc to expect GetDemuxerConfigFromStream() always return true in when config changes. On 2014/05/08 20:05:25, wolenetz wrote: > On 2014/05/08 17:48:37, qinmin wrote: > > The crash will crash the browser process, probably a MEDIA_CODEC_ERROR will be > > better. > > > > On 2014/05/08 17:41:47, wolenetz wrote: > > > nit: I think this is ok, but we would miss learning about broken demuxer > that > > is > > > incorrectly signalling config change without providing config. Would > > > MEDIA_CODEC_ERROR be better? I liked the previous CHECK because it would > > > explicitly crash on this incorrect demuxer behavior. wdyt? > > > > nit: Can we do a CHECK, then, in media_source_delegate, to get crash for this > situation emitted there? This crash should not occur, so if it does, it may be > low-frequency and MEDIA_CODEC_ERROR to a few users might not get attention > directed to getting a fix. wdyt?
On 2014/05/08 20:50:06, qinmin wrote: > https://codereview.chromium.org/257323003/diff/100001/media/base/android/medi... > File media/base/android/media_decoder_job.cc (right): > > https://codereview.chromium.org/257323003/diff/100001/media/base/android/medi... > media/base/android/media_decoder_job.cc:134: MEDIA_CODEC_OK, > Sounds ok to me. modified the media_source_delegate.cc to expect > GetDemuxerConfigFromStream() always return true in when config changes. > > On 2014/05/08 20:05:25, wolenetz wrote: > > On 2014/05/08 17:48:37, qinmin wrote: > > > The crash will crash the browser process, probably a MEDIA_CODEC_ERROR will > be > > > better. > > > > > > On 2014/05/08 17:41:47, wolenetz wrote: > > > > nit: I think this is ok, but we would miss learning about broken demuxer > > that > > > is > > > > incorrectly signalling config change without providing config. Would > > > > MEDIA_CODEC_ERROR be better? I liked the previous CHECK because it would > > > > explicitly crash on this incorrect demuxer behavior. wdyt? > > > > > > > nit: Can we do a CHECK, then, in media_source_delegate, to get crash for this > > situation emitted there? This crash should not occur, so if it does, it may be > > low-frequency and MEDIA_CODEC_ERROR to a few users might not get attention > > directed to getting a fix. wdyt? Thanks Min (still l g t m)
Sorry for the delay. I just made a last pass and ipc security lgtm.
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/257323003/150001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/257323003/150001
Message was sent while issue was closed.
Change committed as 269614 |
