Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2896193002:
v4l2_vda: Adjust the DVLOG levels. (Closed)
Descriptionv4l2_vda: Adjust the DVLOG levels.
We are going to enabling the log in our testing. First step is to
make sure the verbose level meets out expectation.
Basically, we use
1 for errors.
2 for things like initialization, change of resolution, reset, etc.
4 was per frame info.
3 everything between 2 and 4.
BUG=chromium:718334
TEST=build on veyron_minnie.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2896193002
Cr-Commit-Position: refs/heads/master@{#479290}
Committed: https://chromium.googlesource.com/chromium/src/+/7ff2c4bb232deb1cc6aed58aa74996d81d34e098
Patch Set 1 #
Total comments: 37
Patch Set 2 : Address Pawel's comments #Patch Set 3 : Change DVLOG to VLOG for level <= 2 #Messages
Total messages: 19 (13 generated)
Description was changed from ========== v4l2_vda: Adjust the DVLOG levels. We are going to enabling the log in our testing. First step is to make sure the verbose level meets out expectation. Basically, we use 1 for errors. 2 for things like initialization, change of resolution, reset, etc. 4 was per frame info. 3 everything between 2 and 4. BUG=chromium:718334 TEST=build on veyron_minnie. ========== to ========== v4l2_vda: Adjust the DVLOG levels. We are going to enabling the log in our testing. First step is to make sure the verbose level meets out expectation. Basically, we use 1 for errors. 2 for things like initialization, change of resolution, reset, etc. 4 was per frame info. 3 everything between 2 and 4. BUG=chromium:718334 TEST=build on veyron_minnie. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
owenlin@chromium.org changed reviewers: + kcwu@chromium.org, posciak@chromium.org, wuchengli@chromium.org
Please see if the levels meet your expectation. I will change DVLOG(x) to VLOG(x) for x <= 2 afterwards.
https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:442: DVLOGF(1) << "Change state to kDecoding"; 2 or 3? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:613: DVLOGF(3) << "Original egl_image_size=" << egl_image_size_.ToString() 2? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1075: DVLOGF(3); 4? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1101: DVLOGF(2) << "stalled for input buffers"; 3, this is a very frequent print that doesn't give much information, just that we have processed all pending inputs. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1139: DVLOGF(2); I think this is actually a part of each frame task, not the flush sequence, so should be 4? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1167: DVLOGF(2) << "submitting input_id=" << input_record.input_id; Ditto. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1221: DVLOG(3) << "ServiceDeviceTask(): buffer counts: DEC[" This is per frame, but actually quite useful at 3 I'd say, as the only per-frame output above 4. Perhaps we should keep it at 3. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1222: << decoder_input_queue_.size() << "->" << input_ready_queue_.size() Could we keep the original formatting please, I feel it's more readable? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1236: DVLOGF(3); 4? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1311: DVLOGF(3); 2? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1319: DVLOGF(3) << "got resolution change event."; 2? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1331: DVLOGF(3); Perhaps 4? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1421: DVLOGF(3) << "Dequeue output buffer: dqbuf index=" << dqbuf.index Perhaps 4? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1456: DVLOGF(3); Perhaps 4? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1478: DVLOGF(3) << "enqueued input_id=" << input_record.input_id Perhaps 4? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1488: DVLOGF(3) << "buffer " << buffer; Perhaps 4? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1521: DVLOGF(3) << "qbuf.index=" << qbuf.index; Perhaps 4? https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1990: DVLOGF(3); 4 perhaps?
Description was changed from ========== v4l2_vda: Adjust the DVLOG levels. We are going to enabling the log in our testing. First step is to make sure the verbose level meets out expectation. Basically, we use 1 for errors. 2 for things like initialization, change of resolution, reset, etc. 4 was per frame info. 3 everything between 2 and 4. BUG=chromium:718334 TEST=build on veyron_minnie. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== v4l2_vda: Adjust the DVLOG levels. We are going to enabling the log in our testing. First step is to make sure the verbose level meets out expectation. Basically, we use 1 for errors. 2 for things like initialization, change of resolution, reset, etc. 4 was per frame info. 3 everything between 2 and 4. BUG=chromium:718334 TEST=build on veyron_minnie. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
wuchengli@chromium.org changed reviewers: - wuchengli@chromium.org
https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:442: DVLOGF(1) << "Change state to kDecoding"; On 2017/05/23 09:49:21, Pawel Osciak wrote: > 2 or 3? Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:613: DVLOGF(3) << "Original egl_image_size=" << egl_image_size_.ToString() On 2017/05/23 09:49:21, Pawel Osciak wrote: > 2? Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1075: DVLOGF(3); On 2017/05/23 09:49:22, Pawel Osciak wrote: > 4? Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1101: DVLOGF(2) << "stalled for input buffers"; On 2017/05/23 09:49:21, Pawel Osciak wrote: > 3, this is a very frequent print that doesn't give much information, just that > we have processed all pending inputs. Thanks. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1139: DVLOGF(2); On 2017/05/23 09:49:22, Pawel Osciak wrote: > I think this is actually a part of each frame task, not the flush sequence, so > should be 4? Ah, Right. I was confused with Flush. It is not about Flush(), but sending input to decoder. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1167: DVLOGF(2) << "submitting input_id=" << input_record.input_id; On 2017/05/23 09:49:21, Pawel Osciak wrote: > Ditto. Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1221: DVLOG(3) << "ServiceDeviceTask(): buffer counts: DEC[" On 2017/05/23 09:49:21, Pawel Osciak wrote: > This is per frame, but actually quite useful at 3 I'd say, as the only per-frame > output above 4. Perhaps we should keep it at 3. Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1222: << decoder_input_queue_.size() << "->" << input_ready_queue_.size() On 2017/05/23 09:49:21, Pawel Osciak wrote: > Could we keep the original formatting please, I feel it's more readable? OK. But it is the result of "git cl format", we will need to keep the format manually after every "cl format". https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1236: DVLOGF(3); On 2017/05/23 09:49:21, Pawel Osciak wrote: > 4? Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1311: DVLOGF(3); On 2017/05/23 09:49:21, Pawel Osciak wrote: > 2? Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1319: DVLOGF(3) << "got resolution change event."; On 2017/05/23 09:49:21, Pawel Osciak wrote: > 2? Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1331: DVLOGF(3); On 2017/05/23 09:49:21, Pawel Osciak wrote: > Perhaps 4? Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1421: DVLOGF(3) << "Dequeue output buffer: dqbuf index=" << dqbuf.index On 2017/05/23 09:49:22, Pawel Osciak wrote: > Perhaps 4? Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1456: DVLOGF(3); On 2017/05/23 09:49:22, Pawel Osciak wrote: > Perhaps 4? Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1478: DVLOGF(3) << "enqueued input_id=" << input_record.input_id On 2017/05/23 09:49:21, Pawel Osciak wrote: > Perhaps 4? Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1488: DVLOGF(3) << "buffer " << buffer; On 2017/05/23 09:49:21, Pawel Osciak wrote: > Perhaps 4? Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1521: DVLOGF(3) << "qbuf.index=" << qbuf.index; On 2017/05/23 09:49:21, Pawel Osciak wrote: > Perhaps 4? Done. https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1990: DVLOGF(3); On 2017/05/23 09:49:21, Pawel Osciak wrote: > 4 perhaps? Done.
lgtm, thanks! https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2896193002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:1222: << decoder_input_queue_.size() << "->" << input_ready_queue_.size() On 2017/06/02 08:51:43, Owen Lin wrote: > On 2017/05/23 09:49:21, Pawel Osciak wrote: > > Could we keep the original formatting please, I feel it's more readable? > > OK. But it is the result of "git cl format", we will need to keep the format > manually after every "cl format". Yes, unfortunately.
The CQ bit was checked by posciak@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.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by owenlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2896193002/#ps60001 (title: "Change DVLOG to VLOG for level <= 2")
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": 60001, "attempt_start_ts": 1497410222352390,
"parent_rev": "a2205e65faf366cf1748e6a72c4a41188b07e05b", "commit_rev":
"7ff2c4bb232deb1cc6aed58aa74996d81d34e098"}
Message was sent while issue was closed.
Description was changed from ========== v4l2_vda: Adjust the DVLOG levels. We are going to enabling the log in our testing. First step is to make sure the verbose level meets out expectation. Basically, we use 1 for errors. 2 for things like initialization, change of resolution, reset, etc. 4 was per frame info. 3 everything between 2 and 4. BUG=chromium:718334 TEST=build on veyron_minnie. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== v4l2_vda: Adjust the DVLOG levels. We are going to enabling the log in our testing. First step is to make sure the verbose level meets out expectation. Basically, we use 1 for errors. 2 for things like initialization, change of resolution, reset, etc. 4 was per frame info. 3 everything between 2 and 4. BUG=chromium:718334 TEST=build on veyron_minnie. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2896193002 Cr-Commit-Position: refs/heads/master@{#479290} Committed: https://chromium.googlesource.com/chromium/src/+/7ff2c4bb232deb1cc6aed58aa749... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7ff2c4bb232deb1cc6aed58aa749... |
