Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2617823003:
[Chromecast] Make buffering logging less confusing (Closed)
Description[Chromecast] Make buffering logging less confusing
BUG=
Review-Url: https://codereview.chromium.org/2617823003
Cr-Commit-Position: refs/heads/master@{#441882}
Committed: https://chromium.googlesource.com/chromium/src/+/b47d881a6fb62661e9e4319d2a693897835c8acc
Patch Set 1 #
Total comments: 1
Patch Set 2 : change back to CMALOG #Messages
Total messages: 17 (5 generated)
kmackay@chromium.org changed reviewers: + alokp@chromium.org, halliwell@chromium.org
lgtm % nit https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... chromecast/media/cma/pipeline/media_pipeline_impl.cc:355: LOG(INFO) << __FUNCTION__ << " is_buffering=" << is_buffering; We should continue to use VLOG here.
On 2017/01/05 23:57:38, alokp wrote: > lgtm % nit > > https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... > File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): > > https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... > chromecast/media/cma/pipeline/media_pipeline_impl.cc:355: LOG(INFO) << > __FUNCTION__ << " is_buffering=" << is_buffering; > We should continue to use VLOG here. Disagree, we should have some sort of log indicating that buffering is occurring.
On 2017/01/05 23:58:14, kmackay wrote: > On 2017/01/05 23:57:38, alokp wrote: > > lgtm % nit > > > > > https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... > > File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): > > > > > https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... > > chromecast/media/cma/pipeline/media_pipeline_impl.cc:355: LOG(INFO) << > > __FUNCTION__ << " is_buffering=" << is_buffering; > > We should continue to use VLOG here. > > Disagree, we should have some sort of log indicating that buffering is > occurring. I agree, but what is wrong with VLOG? It is captured on the devices and does not spam the developers.
On 2017/01/06 00:00:54, alokp wrote: > On 2017/01/05 23:58:14, kmackay wrote: > > On 2017/01/05 23:57:38, alokp wrote: > > > lgtm % nit > > > > > > > > > https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... > > > File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... > > > chromecast/media/cma/pipeline/media_pipeline_impl.cc:355: LOG(INFO) << > > > __FUNCTION__ << " is_buffering=" << is_buffering; > > > We should continue to use VLOG here. > > > > Disagree, we should have some sort of log indicating that buffering is > > occurring. > > I agree, but what is wrong with VLOG? It is captured on the devices and does not > spam the developers. Rest of the file uses CMALOG macro, should do the same here for consistency.
On 2017/01/06 00:02:53, halliwell wrote: > On 2017/01/06 00:00:54, alokp wrote: > > On 2017/01/05 23:58:14, kmackay wrote: > > > On 2017/01/05 23:57:38, alokp wrote: > > > > lgtm % nit > > > > > > > > > > > > > > https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... > > > > File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... > > > > chromecast/media/cma/pipeline/media_pipeline_impl.cc:355: LOG(INFO) << > > > > __FUNCTION__ << " is_buffering=" << is_buffering; > > > > We should continue to use VLOG here. > > > > > > Disagree, we should have some sort of log indicating that buffering is > > > occurring. > > > > I agree, but what is wrong with VLOG? It is captured on the devices and does > not > > spam the developers. > > Rest of the file uses CMALOG macro, should do the same here for consistency. It already was CMALOG, but in feedback reports there is no logging. So if we leave it as CMALOG, it will look in the logs as if the music is just pausing for no reason.
On 2017/01/06 01:23:16, kmackay wrote: > On 2017/01/06 00:02:53, halliwell wrote: > > On 2017/01/06 00:00:54, alokp wrote: > > > On 2017/01/05 23:58:14, kmackay wrote: > > > > On 2017/01/05 23:57:38, alokp wrote: > > > > > lgtm % nit > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... > > > > > File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... > > > > > chromecast/media/cma/pipeline/media_pipeline_impl.cc:355: LOG(INFO) << > > > > > __FUNCTION__ << " is_buffering=" << is_buffering; > > > > > We should continue to use VLOG here. > > > > > > > > Disagree, we should have some sort of log indicating that buffering is > > > > occurring. > > > > > > I agree, but what is wrong with VLOG? It is captured on the devices and does > > not > > > spam the developers. > > > > Rest of the file uses CMALOG macro, should do the same here for consistency. > > It already was CMALOG, but in feedback reports there is no logging. So if we > leave it as CMALOG, it will look in the logs as if the music is just pausing for > no reason. If there is no logging in feedback reports, then this is a bug in process.json. AFAIK we run cast_shell with --vmodule=*/chromecast/media/*=2.
On 2017/01/06 01:29:44, alokp wrote: > On 2017/01/06 01:23:16, kmackay wrote: > > On 2017/01/06 00:02:53, halliwell wrote: > > > On 2017/01/06 00:00:54, alokp wrote: > > > > On 2017/01/05 23:58:14, kmackay wrote: > > > > > On 2017/01/05 23:57:38, alokp wrote: > > > > > > lgtm % nit > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... > > > > > > File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2617823003/diff/1/chromecast/media/cma/pipeli... > > > > > > chromecast/media/cma/pipeline/media_pipeline_impl.cc:355: LOG(INFO) << > > > > > > __FUNCTION__ << " is_buffering=" << is_buffering; > > > > > > We should continue to use VLOG here. > > > > > > > > > > Disagree, we should have some sort of log indicating that buffering is > > > > > occurring. > > > > > > > > I agree, but what is wrong with VLOG? It is captured on the devices and > does > > > not > > > > spam the developers. > > > > > > Rest of the file uses CMALOG macro, should do the same here for consistency. > > > > It already was CMALOG, but in feedback reports there is no logging. So if we > > leave it as CMALOG, it will look in the logs as if the music is just pausing > for > > no reason. > > If there is no logging in feedback reports, then this is a bug in process.json. > AFAIK we run cast_shell with --vmodule=*/chromecast/media/*=2. Is this perhaps missing from audio process.json? I see this logging all the time in video builds.
> Is this perhaps missing from audio process.json? I see this logging all the > time in video builds. OK, I changed back to CMALOG (but keep the NOTIMPLEMENTED). I'll check the various process.json.
On 2017/01/06 05:21:36, kmackay wrote: > > Is this perhaps missing from audio process.json? I see this logging all the > > time in video builds. > > OK, I changed back to CMALOG (but keep the NOTIMPLEMENTED). I'll check the > various process.json. lgtm
The CQ bit was checked by kmackay@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alokp@chromium.org Link to the patchset: https://codereview.chromium.org/2617823003/#ps20001 (title: "change back to CMALOG")
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": 20001, "attempt_start_ts": 1483681164589090,
"parent_rev": "49b2b8a8727853391ce1f38de8044cfd4678ed1f", "commit_rev":
"b47d881a6fb62661e9e4319d2a693897835c8acc"}
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Make buffering logging less confusing BUG= ========== to ========== [Chromecast] Make buffering logging less confusing BUG= Review-Url: https://codereview.chromium.org/2617823003 Cr-Commit-Position: refs/heads/master@{#441882} Committed: https://chromium.googlesource.com/chromium/src/+/b47d881a6fb62661e9e4319d2a69... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b47d881a6fb62661e9e4319d2a69... |
