Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2124073006:
Remove VideoCaptureDeviceClient buffer warning logs (Closed)
|
Created:
4 years, 5 months ago by emircan Modified:
4 years, 5 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove VideoCaptureDeviceClient buffer warning logs
VideoCaptureDeviceClient buffer warnings show that capture is
stopped. In most of the capture related errors, capture stops
and we print this warning for each following frame. As a result,
it is much harder to read through the error logs, such as below
as we print this line hundreds of times.
This CL addresses this issue by removing the logs and replacing with
a DCHECK that comes in after consistent errors.
https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26646/steps/browser_tests/logs/stdio
Committed: https://crrev.com/bcd0585fc2b25d2f87d15982e7aeae4b8364888a
Cr-Commit-Position: refs/heads/master@{#405274}
Patch Set 1 : #Patch Set 2 : Move to base/logging #Patch Set 3 : Move to media/base. #Patch Set 4 : Turn into DCHECK. #
Total comments: 5
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 38 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== decrease BUG= ========== to ========== Decrease VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by making sure that we print the first N warnings followed by the every Nth warning coming after. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... ==========
emircan@chromium.org changed reviewers: + kjellander@webrtc.org, mcasas@chromium.org
PTAL. mcasas@ for OWNERS review. kjellander@ take note as I remember you were annoyed by the amount of these warnings too.
Description was changed from ========== Decrease VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by making sure that we print the first N warnings followed by the every Nth warning coming after. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... ========== to ========== Decrease VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by making sure that we print the first N warnings followed by the every Nth warning coming after, where N=20. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... ==========
Drive-by comment: I feel a little uneasy about the clutter of having to add #ifs at 4 locations for this mechanism. Outputting log messages only every n-th time seems to be a commonly useful thing. How about we add some macros for it in logging.h and move all the #ifs there. Usage could look something like this: DLOG_INTERVAL(WARNING, 20) << "Log message output only every 20th time". Question: Is it really necessary to have the |warning_counter_| be a member of VideoCaptureDeviceClient? Unless we have situations where there are multiple instances of VideoCaptureDeviceClient and we need each of them to keep track of their own |warning_counter_|, we could use a local static variable declaration and put it inside the DLOG_INTERVAL() macro definition. This way we would not need to add any code to the header and constructor.
chfremer@ thanks for the input. It would be a useful macro, especially for video-capture related code as we print most of the logs for each frame. I added a LOG_INTERVAL() as you suggested. However, I refrained from declaring the counter inside the macro as "static" would be wrong here (in case of multiple instances). We can build on this macro later when the use-case is there.
emircan@chromium.org changed reviewers: + chfremer@chromium.org - kjellander@webrtc.org
Description was changed from ========== Decrease VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by making sure that we print the first N warnings followed by the every Nth warning coming after, where N=20. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... ========== to ========== Decrease VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by making sure that we print the first M warnings followed by the every Nth warning coming after. I added a logging macro DLOG_INTERVAL_COUNTER() for this purpose. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... ==========
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
emircan@chromium.org changed reviewers: + thakis@chromium.org
thakis@ for OWNERS review on base/logging*
have you seen the text in base/OWNERS? do we really need more stuff for this in base? just put a bool "wrote log?" at the calling site -- logging.h is already way too long. actually, probably better still just delete the error log completely -- is it ever useful? in general, we strongly discourage logging in chromium. regular users don't look at logs, so they fill up disks (and buildbot test run output) without any benefit. if it's an important error that shouldn't happen, CHECK(false) instead, then you'll get crash logs if it unexpectedly does happen.
On 2016/07/12 01:13:51, Nico wrote: > have you seen the text in base/OWNERS? do we really need more stuff for this in > base? just put a bool "wrote log?" at the calling site -- logging.h is already > way too long. > > actually, probably better still just delete the error log completely -- is it > ever useful? in general, we strongly discourage logging in chromium. regular > users don't look at logs, so they fill up disks (and buildbot test run output) > without any benefit. if it's an important error that shouldn't happen, > CHECK(false) instead, then you'll get crash logs if it unexpectedly does happen. I just read through. Considering the scope, it makes sense not to add it to base. I am moving it to /media. I think this error is still useful for developers, and it makes sense to keep it. It notifies with a DLOG that the captured frame is dropped due to some slow element in the pipeline that does not give the buffers back. Couple instances of this error can be expected, but when something else breaks, we flood the logs with this. It happens a lot with bots and fuzzers. Moreover, any error that is based on captured frame has this problems as we go through ~30 frames per second. If we have a common DLOG_WARNING() we can use it in many places as chfremer@ pointed out, such as https://bugs.chromium.org/p/chromium/issues/detail?id=623601.
emircan@chromium.org changed reviewers: + chcunningham@chromium.org
chcunningham@ for OWNERS review on media/base/media_util.h. Let me know if it would be a good fit here.
On 2016/07/12 22:21:34, emircan wrote: > On 2016/07/12 01:13:51, Nico wrote: > > have you seen the text in base/OWNERS? do we really need more stuff for this > in > > base? just put a bool "wrote log?" at the calling site -- logging.h is already > > way too long. > > > > actually, probably better still just delete the error log completely -- is it > > ever useful? in general, we strongly discourage logging in chromium. regular > > users don't look at logs, so they fill up disks (and buildbot test run output) > > without any benefit. if it's an important error that shouldn't happen, > > CHECK(false) instead, then you'll get crash logs if it unexpectedly does > happen. > > I just read through. Considering the scope, it makes sense not to add it to > base. I am moving it to /media. > > I think this error is still useful for developers, and it makes sense to keep > it. It notifies with a DLOG that the captured frame is dropped due to some slow > element in the pipeline that does not give the buffers back. Couple instances of > this error can be expected Then the expected instances shouldn't log :-) It's a real problem that tests log "X might be wrong but then again it might not" to bot stdout -- this is confusing for everyone who doesn't work in the area of the code that's emitting the log. DLOG_INTERVAL seems like the wrong approach to me. Also see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
emircan@chromium.org changed reviewers: - chcunningham@chromium.org
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Description was changed from ========== Decrease VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by making sure that we print the first M warnings followed by the every Nth warning coming after. I added a logging macro DLOG_INTERVAL_COUNTER() for this purpose. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... ========== to ========== Decrease VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by removing the logs and replacing with a DCHECK that comes in after consistent errors. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... ==========
On 2016/07/12 23:18:34, Nico wrote: > Then the expected instances shouldn't log :-) It's a real problem that tests log > "X might be wrong but then again it might not" to bot stdout -- this is > confusing for everyone who doesn't work in the area of the code that's emitting > the log. DLOG_INTERVAL seems like the wrong approach to me. > > Also see > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Ok, I agree on getting rid of logs that would appear every ~5 sec or so. Note that this was a DLOG(ERROR) originally. I am adding a counter and DCHECK so that it would crash if capture is stalled for ~5 seconds. Changing the CL description as well.
Thanks, that looks like a much better change to me (but I'm not an owner here, so I can't approve)
https://codereview.chromium.org/2124073006/diff/180001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/2124073006/diff/180001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:143: DCHECK_LT(dropped_frame_counter_, 150u) I'd recommend defining a constant static const int kMaxDroppedFrames = 150u next to the declaration of |dropped_frame_counter_|. Also, I'd make |drop_frame_counter_| int, since I don't see any particular reason to use sophisticated (and discouraged, because unsigned) data types. Finally, why have a separate debug version of error handling and not just do here a OnError(FROM_HERE, "Too many frames dropped"); ? https://codereview.chromium.org/2124073006/diff/180001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:146: // Failed to reserve I420 output buffer, so drop the frame. nit: Shift l.146 two spaces left.
https://codereview.chromium.org/2124073006/diff/180001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/2124073006/diff/180001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:143: DCHECK_LT(dropped_frame_counter_, 150u) On 2016/07/13 00:31:36, mcasas wrote: > I'd recommend defining a constant > static const int kMaxDroppedFrames = 150u > next to the declaration of |dropped_frame_counter_|. > > Also, I'd make |drop_frame_counter_| int, since > I don't see any particular reason to use sophisticated > (and discouraged, because unsigned) data types. > > Finally, why have a separate debug version of error > handling and not just do here a > OnError(FROM_HERE, "Too many frames dropped"); > ? Changed |dropped_frame_counter_| to int and declared |kMaxDroppedFrames|. This macro gives me the syntactic sugar for int comparison. I will go for CHECK_LT() so that it doesn't go through debug version. https://codereview.chromium.org/2124073006/diff/180001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:146: // Failed to reserve I420 output buffer, so drop the frame. On 2016/07/13 00:31:36, mcasas wrote: > nit: Shift l.146 two spaces left. Done.
https://codereview.chromium.org/2124073006/diff/180001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/2124073006/diff/180001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:143: DCHECK_LT(dropped_frame_counter_, 150u) On 2016/07/13 17:51:14, emircan wrote: > On 2016/07/13 00:31:36, mcasas wrote: > > I'd recommend defining a constant > > static const int kMaxDroppedFrames = 150u > > next to the declaration of |dropped_frame_counter_|. > > > > Also, I'd make |drop_frame_counter_| int, since > > I don't see any particular reason to use sophisticated > > (and discouraged, because unsigned) data types. > > > > Finally, why have a separate debug version of error > > handling and not just do here a > > OnError(FROM_HERE, "Too many frames dropped"); > > ? > > Changed |dropped_frame_counter_| to int and declared |kMaxDroppedFrames|. This > macro gives me the syntactic sugar for int comparison. I will go for CHECK_LT() > so that it doesn't go through debug version. Nvm. I just realized OnError is a call within this function. Switching to that.
On 2016/07/13 18:24:56, emircan wrote: > https://codereview.chromium.org/2124073006/diff/180001/content/browser/render... > File content/browser/renderer_host/media/video_capture_device_client.cc (right): > > https://codereview.chromium.org/2124073006/diff/180001/content/browser/render... > content/browser/renderer_host/media/video_capture_device_client.cc:143: > DCHECK_LT(dropped_frame_counter_, 150u) > On 2016/07/13 17:51:14, emircan wrote: > > On 2016/07/13 00:31:36, mcasas wrote: > > > I'd recommend defining a constant > > > static const int kMaxDroppedFrames = 150u > > > next to the declaration of |dropped_frame_counter_|. > > > > > > Also, I'd make |drop_frame_counter_| int, since > > > I don't see any particular reason to use sophisticated > > > (and discouraged, because unsigned) data types. > > > > > > Finally, why have a separate debug version of error > > > handling and not just do here a > > > OnError(FROM_HERE, "Too many frames dropped"); > > > ? > > > > Changed |dropped_frame_counter_| to int and declared |kMaxDroppedFrames|. This > > macro gives me the syntactic sugar for int comparison. I will go for > CHECK_LT() > > so that it doesn't go through debug version. > > Nvm. I just realized OnError is a call within this function. Switching to that. PS#6 LGTM, please update description.
Description was changed from ========== Decrease VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by removing the logs and replacing with a DCHECK that comes in after consistent errors. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... ========== to ========== Remove VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by removing the logs and replacing with a DCHECK that comes in after consistent errors. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... ==========
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
Message was sent while issue was closed.
Description was changed from ========== Remove VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by removing the logs and replacing with a DCHECK that comes in after consistent errors. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... ========== to ========== Remove VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by removing the logs and replacing with a DCHECK that comes in after consistent errors. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Remove VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by removing the logs and replacing with a DCHECK that comes in after consistent errors. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... ========== to ========== Remove VideoCaptureDeviceClient buffer warning logs VideoCaptureDeviceClient buffer warnings show that capture is stopped. In most of the capture related errors, capture stops and we print this warning for each following frame. As a result, it is much harder to read through the error logs, such as below as we print this line hundreds of times. This CL addresses this issue by removing the logs and replacing with a DCHECK that comes in after consistent errors. https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/26... Committed: https://crrev.com/bcd0585fc2b25d2f87d15982e7aeae4b8364888a Cr-Commit-Position: refs/heads/master@{#405274} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/bcd0585fc2b25d2f87d15982e7aeae4b8364888a Cr-Commit-Position: refs/heads/master@{#405274} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
