Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2638393005:
Media Remoting: Exit remoting only if playback is continuously delayed. (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: Exit remoting only if playback is continuously delayed.
The local time to process the TimeUpdate RPC message is used to detect
whether playback is delayed. Since the RPC message might not be
processed in time, only exits remoting if playback is continously
delayed.
BUG=643964, 684749
Review-Url: https://codereview.chromium.org/2638393005
Cr-Commit-Position: refs/heads/master@{#445136}
Committed: https://chromium.googlesource.com/chromium/src/+/b53feae503c3d2a12f9f07c40dad53229bc8bbde
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed comments. #
Messages
Total messages: 20 (14 generated)
Patchset #1 (id:1) has been deleted
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.
Approach looks good, just some naming issues (PS1): https://codereview.chromium.org/2638393005/diff/20001/media/remoting/remote_r... File media/remoting/remote_renderer_impl.cc (right): https://codereview.chromium.org/2638393005/diff/20001/media/remoting/remote_r... media/remoting/remote_renderer_impl.cc:36: constexpr int kMaxNumPlaybackDelayed = 3; How about |kPlaybackDelayCountThreshold|? https://codereview.chromium.org/2638393005/diff/20001/media/remoting/remote_r... File media/remoting/remote_renderer_impl.h (right): https://codereview.chromium.org/2638393005/diff/20001/media/remoting/remote_r... media/remoting/remote_renderer_impl.h:224: // Records the number that remoting playback is continuously delayed. s/number that/number of consecutive times/ https://codereview.chromium.org/2638393005/diff/20001/media/remoting/remote_r... media/remoting/remote_renderer_impl.h:224: // Records the number that remoting playback is continuously delayed. s/is/was/ https://codereview.chromium.org/2638393005/diff/20001/media/remoting/remote_r... media/remoting/remote_renderer_impl.h:225: int num_playback_delayed_ = 0; How about |times_playback_delayed_|?
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Addressed comments. PTAL again. Thanks! https://codereview.chromium.org/2638393005/diff/20001/media/remoting/remote_r... File media/remoting/remote_renderer_impl.cc (right): https://codereview.chromium.org/2638393005/diff/20001/media/remoting/remote_r... media/remoting/remote_renderer_impl.cc:36: constexpr int kMaxNumPlaybackDelayed = 3; On 2017/01/19 22:00:55, miu wrote: > How about |kPlaybackDelayCountThreshold|? Done. https://codereview.chromium.org/2638393005/diff/20001/media/remoting/remote_r... File media/remoting/remote_renderer_impl.h (right): https://codereview.chromium.org/2638393005/diff/20001/media/remoting/remote_r... media/remoting/remote_renderer_impl.h:224: // Records the number that remoting playback is continuously delayed. On 2017/01/19 22:00:55, miu wrote: > s/number that/number of consecutive times/ Done. https://codereview.chromium.org/2638393005/diff/20001/media/remoting/remote_r... media/remoting/remote_renderer_impl.h:224: // Records the number that remoting playback is continuously delayed. On 2017/01/19 22:00:55, miu wrote: > s/is/was/ Done. https://codereview.chromium.org/2638393005/diff/20001/media/remoting/remote_r... media/remoting/remote_renderer_impl.h:225: int num_playback_delayed_ = 0; On 2017/01/19 22:00:55, miu wrote: > How about |times_playback_delayed_|? Done.
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.
The CQ bit was checked by miu@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484941088431660,
"parent_rev": "c0a67bd73b2f1e8755c46415a23b2792b6eaff6f", "commit_rev":
"b53feae503c3d2a12f9f07c40dad53229bc8bbde"}
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Exit remoting only if playback is continuously delayed. The local time to process the TimeUpdate RPC message is used to detect whether playback is delayed. Since the RPC message might not be processed in time, only exits remoting if playback is continously delayed. BUG=643964 ========== to ========== Media Remoting: Exit remoting only if playback is continuously delayed. The local time to process the TimeUpdate RPC message is used to detect whether playback is delayed. Since the RPC message might not be processed in time, only exits remoting if playback is continously delayed. BUG=643964 Review-Url: https://codereview.chromium.org/2638393005 Cr-Commit-Position: refs/heads/master@{#445136} Committed: https://chromium.googlesource.com/chromium/src/+/b53feae503c3d2a12f9f07c40dad... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b53feae503c3d2a12f9f07c40dad...
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Exit remoting only if playback is continuously delayed. The local time to process the TimeUpdate RPC message is used to detect whether playback is delayed. Since the RPC message might not be processed in time, only exits remoting if playback is continously delayed. BUG=643964 Review-Url: https://codereview.chromium.org/2638393005 Cr-Commit-Position: refs/heads/master@{#445136} Committed: https://chromium.googlesource.com/chromium/src/+/b53feae503c3d2a12f9f07c40dad... ========== to ========== Media Remoting: Exit remoting only if playback is continuously delayed. The local time to process the TimeUpdate RPC message is used to detect whether playback is delayed. Since the RPC message might not be processed in time, only exits remoting if playback is continously delayed. BUG=643964,684749 Review-Url: https://codereview.chromium.org/2638393005 Cr-Commit-Position: refs/heads/master@{#445136} Committed: https://chromium.googlesource.com/chromium/src/+/b53feae503c3d2a12f9f07c40dad... ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
