Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2777583002:
Move getUserMedia finish to "when audio track configured". (Closed)
|
Created:
3 years, 9 months ago by hta - Chromium Modified:
3 years, 8 months ago Reviewers:
Guido Urdaneta CC:
chromium-reviews, mlamouri+watch-content_chromium.org, imcheng+watch_chromium.org, feature-media-reviews_chromium.org, posciak+watch_chromium.org, chfremer+watch_chromium.org, jam, avayvod+watch_chromium.org, darin-cc_chromium.org, jasonroberts+watch_google.com, xjz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove getUserMedia finish to "when audio track configured".
Previously, getUserMedia() would complete as soon as the sources
were initialized - which did not do all the initialization.
This CL will make getUserMedia() return when the track has been
told of its configuration through OnSetFormat() from upstream.
This will allow GetSettings() to read the audio configuration from
the track without worrying about whether the configuration is
propagated or not.
BUG=704918
Review-Url: https://codereview.chromium.org/2777583002
Cr-Commit-Position: refs/heads/master@{#462523}
Committed: https://chromium.googlesource.com/chromium/src/+/a52d69d1877b170684041bed04d28acced85e40d
Patch Set 1 #Patch Set 2 : Ensure callback is called on owner thread #Patch Set 3 : UserMediaClientImplUnittest passes #Patch Set 4 : Lock access to the track callback #Patch Set 5 : Thread-safety on unittest #
Total comments: 10
Patch Set 6 : Review comments #Patch Set 7 : Fix error introduced when addressing review #
Total comments: 1
Messages
Total messages: 46 (35 generated)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Description was changed from ========== Move getUserMedia finish to "when audio track configured". Previously, getUserMedia() would complete as soon as the sources were initialized - which did not do all the initialization. This CL will make getUserMedia() return when the track has been told of its configuration through OnSetFormat() from upstream. STATUS: Compiles, not all tests pass. BUG=704918 ========== to ========== Move getUserMedia finish to "when audio track configured". Previously, getUserMedia() would complete as soon as the sources were initialized - which did not do all the initialization. This CL will make getUserMedia() return when the track has been told of its configuration through OnSetFormat() from upstream. STATUS: Compiles, not all tests pass. BUG=704918 ==========
hta@chromium.org changed reviewers: + guidou@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I think this is ready for review.
Description was changed from ========== Move getUserMedia finish to "when audio track configured". Previously, getUserMedia() would complete as soon as the sources were initialized - which did not do all the initialization. This CL will make getUserMedia() return when the track has been told of its configuration through OnSetFormat() from upstream. STATUS: Compiles, not all tests pass. BUG=704918 ========== to ========== Move getUserMedia finish to "when audio track configured". Previously, getUserMedia() would complete as soon as the sources were initialized - which did not do all the initialization. This CL will make getUserMedia() return when the track has been told of its configuration through OnSetFormat() from upstream. This will allow GetSettings() to read the audio configuration from the track without worrying about whether the configuration is propagated or not. BUG=704918 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_audio_track.cc (right): https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_audio_track.cc:158: DCHECK(format_is_set_); Does format_is_set_ need to be protected by the lock? If so, use #if DCHECK_IS_ON() around the DCHECK. https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:717: // NotifyCurrentRequestInfoOfAudioSourceStarted(source, result, Use a more descriptive comment. Something like: // Do not notify the current request about audio started until ... https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:1299: if (!is_pending && !connected) { Should this be ||? What happens if it is pending but ConnectToTrack failed? https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:1374: // If the start failed, inform the request here. Extend the comment with "Otherwise, wait for ... to notify the request". As is it now, it looks like the notification is ignored if the start is successful. https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... File content/renderer/media/user_media_client_impl_unittest.cc (right): https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... content/renderer/media/user_media_client_impl_unittest.cc:267: class GeneratingAudioSource : public MediaStreamAudioSource { Normally, I would request this class to be defined somewhere else, but in this case it is more consistent with the existing code to define it here.
Better now? https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_audio_track.cc (right): https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_audio_track.cc:158: DCHECK(format_is_set_); On 2017/04/06 13:18:57, Guido Urdaneta wrote: > Does format_is_set_ need to be protected by the lock? > If so, use #if DCHECK_IS_ON() around the DCHECK. I put it on protection; not sure it's required for bools, but better be safe than sorry. I used an accessor to avoid the extra visual noise of #if DCHECK_IS_ON. https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:717: // NotifyCurrentRequestInfoOfAudioSourceStarted(source, result, On 2017/04/06 13:18:57, Guido Urdaneta wrote: > Use a more descriptive comment. Something like: > > // Do not notify the current request about audio started until ... My bad. Deleting the commented-out code instead. https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:1299: if (!is_pending && !connected) { On 2017/04/06 13:18:57, Guido Urdaneta wrote: > Should this be ||? > What happens if it is pending but ConnectToTrack failed? the previous code was if !pending & connected: call with MEDIA_DEVICE_OK if !pending & !connected: call with MEDIA_DEVICE_TRACK_START_FAILURE if pending: do nothing the new code is if pending & connected: set up callback that eventually does MEDIA_DEVICE_OK if !pending & !connected: call with MEDIA_DEVICE_TRACK_FAILURE if !pending: do nothing This can be made more readable. https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... File content/renderer/media/user_media_client_impl_unittest.cc (right): https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... content/renderer/media/user_media_client_impl_unittest.cc:267: class GeneratingAudioSource : public MediaStreamAudioSource { On 2017/04/06 13:18:57, Guido Urdaneta wrote: > Normally, I would request this class to be defined somewhere else, but in this > case it is more consistent with the existing code to define it here. Acknowledged. I thought about merging FailedAtLifeAudioSource and GeneratingAudioSource and making a separate MockAudioSource object with parameters, but then I decided to follow the existing pattern instead.
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_audio_track.cc (right): https://codereview.chromium.org/2777583002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_audio_track.cc:158: DCHECK(format_is_set_); On 2017/04/06 13:18:57, Guido Urdaneta wrote: > Does format_is_set_ need to be protected by the lock? > If so, use #if DCHECK_IS_ON() around the DCHECK. You solved it in a nicer way :)
The CQ bit was unchecked by hta@chromium.org
The CQ bit was checked by hta@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org Link to the patchset: https://codereview.chromium.org/2777583002/#ps120001 (title: "Fix error introduced when addressing review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2777583002/diff/120001/content/renderer/media... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/2777583002/diff/120001/content/renderer/media... content/renderer/media/user_media_client_impl.cc:1295: if (!is_pending && !connected) { Shouldn't it fail if is_pending, but !connected?
On 2017/04/06 15:48:40, Guido Urdaneta wrote: > https://codereview.chromium.org/2777583002/diff/120001/content/renderer/media... > File content/renderer/media/user_media_client_impl.cc (right): > > https://codereview.chromium.org/2777583002/diff/120001/content/renderer/media... > content/renderer/media/user_media_client_impl.cc:1295: if (!is_pending && > !connected) { > Shouldn't it fail if is_pending, but !connected? lgtm after discussing offline. disregard the last question.
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491493060116380,
"parent_rev": "72b0aa102b73e63011870f69bfac366af8686ace", "commit_rev":
"a52d69d1877b170684041bed04d28acced85e40d"}
Message was sent while issue was closed.
Description was changed from ========== Move getUserMedia finish to "when audio track configured". Previously, getUserMedia() would complete as soon as the sources were initialized - which did not do all the initialization. This CL will make getUserMedia() return when the track has been told of its configuration through OnSetFormat() from upstream. This will allow GetSettings() to read the audio configuration from the track without worrying about whether the configuration is propagated or not. BUG=704918 ========== to ========== Move getUserMedia finish to "when audio track configured". Previously, getUserMedia() would complete as soon as the sources were initialized - which did not do all the initialization. This CL will make getUserMedia() return when the track has been told of its configuration through OnSetFormat() from upstream. This will allow GetSettings() to read the audio configuration from the track without worrying about whether the configuration is propagated or not. BUG=704918 Review-Url: https://codereview.chromium.org/2777583002 Cr-Commit-Position: refs/heads/master@{#462523} Committed: https://chromium.googlesource.com/chromium/src/+/a52d69d1877b170684041bed04d2... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/a52d69d1877b170684041bed04d2...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2803183002/ by foolip@chromium.org. The reason for reverting is: The added DCHECK(format_is_set()) is failing in MediaStreamTrack-getSettings.html BUG=709188. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
