Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2648303003:
Media Remoting: Don't exit remoting if no enough data buffered. (Closed)
|
Created:
3 years, 11 months ago by xjz Modified:
3 years, 11 months ago Reviewers:
miu CC:
chromium-reviews, chromoting-reviews_chromium.org, feature-media-reviews_chromium.org, apacible+watch_chromium.org, xjz+watch_chromium.org, erickung+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia Remoting: Don't exit remoting if no enough data buffered.
When there is not enough data buffered for rendering, remote playback
may be paused/delayed. Don't exit remoting in this case.
BUG=684078
Review-Url: https://codereview.chromium.org/2648303003
Cr-Commit-Position: refs/heads/master@{#445542}
Committed: https://chromium.googlesource.com/chromium/src/+/551abe8c8bf1d5276dd03e98615ceb928597d719
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comments. #
Messages
Total messages: 22 (12 generated)
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
xjz@chromium.org changed reviewers: + miu@chromium.org
PTAL
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.
lgtm % one thing that you may or may not want to change: https://codereview.chromium.org/2648303003/diff/1/media/remoting/remote_rende... File media/remoting/remote_renderer_impl.h (right): https://codereview.chromium.org/2648303003/diff/1/media/remoting/remote_rende... media/remoting/remote_renderer_impl.h:235: bool is_waiting_for_buffering_ = false; This is probably immaterial, but this starts out as true (at construction time), right?
Thanks for reviewing. https://codereview.chromium.org/2648303003/diff/1/media/remoting/remote_rende... File media/remoting/remote_renderer_impl.h (right): https://codereview.chromium.org/2648303003/diff/1/media/remoting/remote_rende... media/remoting/remote_renderer_impl.h:235: bool is_waiting_for_buffering_ = false; On 2017/01/23 21:01:48, miu wrote: > This is probably immaterial, but this starts out as true (at construction time), > right? Done. Thanks!
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org Link to the patchset: https://codereview.chromium.org/2648303003/#ps20001 (title: "Addressed comments.")
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 miu@chromium.org
On 2017/01/23 21:22:28, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Before commit, could you open a crbug for this issue and set BUG= to it? This'll make the merge request process go smoothly.
Description was changed from ========== Media Remoting: Don't exit remoting if no enough data buffered. When there is not enough data buffered for rendering, remote playback may be paused/delayed. Don't exit remoting in this case. BUG=643964 ========== to ========== Media Remoting: Don't exit remoting if no enough data buffered. When there is not enough data buffered for rendering, remote playback may be paused/delayed. Don't exit remoting in this case. BUG=684078 ==========
On 2017/01/23 21:32:30, miu wrote: > On 2017/01/23 21:22:28, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > Before commit, could you open a crbug for this issue and set BUG= to it? This'll > make the merge request process go smoothly. Done.
The CQ bit was checked by xjz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/23 21:39:06, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... just put here as a note that the CL is true because renderer only can reports BUFFERING_HAVE_ENOUGH because WebMediaPlayerImpl doesn't handle other value http://crbug.com/144683.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485207494640700,
"parent_rev": "aaa433c5e54642be09cbdec198fd44f880e80c7b", "commit_rev":
"551abe8c8bf1d5276dd03e98615ceb928597d719"}
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Don't exit remoting if no enough data buffered. When there is not enough data buffered for rendering, remote playback may be paused/delayed. Don't exit remoting in this case. BUG=684078 ========== to ========== Media Remoting: Don't exit remoting if no enough data buffered. When there is not enough data buffered for rendering, remote playback may be paused/delayed. Don't exit remoting in this case. BUG=684078 Review-Url: https://codereview.chromium.org/2648303003 Cr-Commit-Position: refs/heads/master@{#445542} Committed: https://chromium.googlesource.com/chromium/src/+/551abe8c8bf1d5276dd03e98615c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/551abe8c8bf1d5276dd03e98615c...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2648423004/ by xjz@chromium.org. The reason for reverting is: I just realized that this BufferingState is reported from receiver side. So the BUFFERING_HAVE_NOTHING state could also be caused by insufficient bandwidth condition, and should exit remoting. Revert this CL for now, and will re-think this issue.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
