Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1746443002:
Use Timer in TimedCanvasDrawListener (Closed)
DescriptionUse Timer in TimedCanvasDrawListener
In the initial implementation, we used postDelayedTask() for
repeating tasks in TimedCanvasDrawListener. However, this
would introduce drift, jitter and a possible memory leak if tasks
in flight prevent the class from being GC'ed. This CL uses
UnthrottledTimer instead for the same purposes, which addresses
drift and memory leak issues.
BUG=589974
TEST=It is very difficult to write a non-flaky Layout Test for this.
Therefore, we went with browser test here https://codereview.chromium.org/1692773002/.
Committed: https://crrev.com/91f06f9ef435b692f4ed020c2cb619f7e2201848
Cr-Commit-Position: refs/heads/master@{#378616}
Patch Set 1 : #
Total comments: 15
Patch Set 2 : #
Messages
Total messages: 21 (11 generated)
Description was changed from ========== timer instead of posttask. BUG=589974 ========== to ========== Use Timer in TimedCanvasDrawListener In the initial implementation, we used postDelayedTask() for repeating tasks in TimedCanvasDrawListener. However, this would intorduce a jitter, and a possible memory leak when class goes out of scope. This CL uses UnthrottledTimer instead. BUG=589974 TEST=It is very difficult to write a non-flaky Layout Test for this. Therefore, we went with browser test here https://codereview.chromium.org/1692773002/. ==========
Description was changed from ========== Use Timer in TimedCanvasDrawListener In the initial implementation, we used postDelayedTask() for repeating tasks in TimedCanvasDrawListener. However, this would intorduce a jitter, and a possible memory leak when class goes out of scope. This CL uses UnthrottledTimer instead. BUG=589974 TEST=It is very difficult to write a non-flaky Layout Test for this. Therefore, we went with browser test here https://codereview.chromium.org/1692773002/. ========== to ========== Use Timer in TimedCanvasDrawListener In the initial implementation, we used postDelayedTask() for repeating tasks in TimedCanvasDrawListener. However, this would introduce a jitter, and a possible memory leak when class goes out of scope. This CL uses UnthrottledTimer instead. BUG=589974 TEST=It is very difficult to write a non-flaky Layout Test for this. Therefore, we went with browser test here https://codereview.chromium.org/1692773002/. ==========
Patchset #1 (id:1) has been deleted
emircan@chromium.org changed reviewers: + mcasas@chromium.org, philipj@opera.com
PTAL.
emircan@chromium.org changed reviewers: + hiroshige@chromium.org
Description was changed from ========== Use Timer in TimedCanvasDrawListener In the initial implementation, we used postDelayedTask() for repeating tasks in TimedCanvasDrawListener. However, this would introduce a jitter, and a possible memory leak when class goes out of scope. This CL uses UnthrottledTimer instead. BUG=589974 TEST=It is very difficult to write a non-flaky Layout Test for this. Therefore, we went with browser test here https://codereview.chromium.org/1692773002/. ========== to ========== Use Timer in TimedCanvasDrawListener In the initial implementation, we used postDelayedTask() for repeating tasks in TimedCanvasDrawListener. However, this would introduce a jitter, and a possible memory leak if tasks in flight prevent the class from being GC'ed. This CL uses UnthrottledTimer instead for the same purposes. BUG=589974 TEST=It is very difficult to write a non-flaky Layout Test for this. Therefore, we went with browser test here https://codereview.chromium.org/1692773002/. ==========
LGTM, but would like mcasas@ to always have the final word as the module owner. https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp (right): https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:26: listener->m_requestFrameTimer.startRepeating(listener->m_frameInterval, BLINK_FROM_HERE); Yeah, judging by TimerBase::runInternal this should do the trick when it comes to drift, but it actually won't solve the problem of frame drops due to time jitter. It does solve the memory leak, though. https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.h (right): https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.h:17: Did you intentionally add this blank line? You didn't add one before private: below.
LGTM w/ nits https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp (right): https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:15: , m_frameInterval(1 / frameRate) nit: perhaps unnecessary, but I suggest s/1/1.0/ so it's super clear that those are floating point values. https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:38: m_frameCaptureRequested = true; nit: Perhaps something for down the line, but you could add some dev provision to check for how much deviation/jitter we're actually getting versus expected, and maybe add some logging of sorts if, say, it deviates > 10% ? I'm happy with a crbug too, if any. https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.h (right): https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.h:26: void requestFrameTimerFired(Timer<TimedCanvasDrawListener>*); nit: Can you add a comment and say this complies to a TimerFiredFunction signature?
https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp (right): https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:15: , m_frameInterval(1 / frameRate) On 2016/02/29 17:00:53, mcasas wrote: > nit: perhaps unnecessary, but I suggest > s/1/1.0/ so it's super clear that those are > floating point values. https://www.chromium.org/blink/coding-style#TOC-Floating-point-literals says to only use it to force floating point math. I can't say it's obvious to me that it isn't needed here, but if it is, then the existing code would be very broken as m_frameInterval would be zero.
https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp (right): https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:15: , m_frameInterval(1 / frameRate) On 2016/02/29 17:39:49, philipj_UTC7 wrote: > On 2016/02/29 17:00:53, mcasas wrote: > > nit: perhaps unnecessary, but I suggest > > s/1/1.0/ so it's super clear that those are > > floating point values. > > https://www.chromium.org/blink/coding-style#TOC-Floating-point-literals says to > only use it to force floating point math. I can't say it's obvious to me that it > isn't needed here, but if it is, then the existing code would be very broken as > m_frameInterval would be zero. To clarify: it isn't needed (the code is correct w |1| since |frameRate| is double), I just find it easier to read :) (kind of self documenting if you want).
lgtm with a nit. https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp (right): https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:8: #include "public/platform/WebTaskRunner.h" nit: probably can we remove #include for Platform.h and WebTaskRunner.h?
Description was changed from ========== Use Timer in TimedCanvasDrawListener In the initial implementation, we used postDelayedTask() for repeating tasks in TimedCanvasDrawListener. However, this would introduce a jitter, and a possible memory leak if tasks in flight prevent the class from being GC'ed. This CL uses UnthrottledTimer instead for the same purposes. BUG=589974 TEST=It is very difficult to write a non-flaky Layout Test for this. Therefore, we went with browser test here https://codereview.chromium.org/1692773002/. ========== to ========== Use Timer in TimedCanvasDrawListener In the initial implementation, we used postDelayedTask() for repeating tasks in TimedCanvasDrawListener. However, this would introduce drift, jitter and a possible memory leak if tasks in flight prevent the class from being GC'ed. This CL uses UnthrottledTimer instead for the same purposes, which addresses drift and memory leak issues. BUG=589974 TEST=It is very difficult to write a non-flaky Layout Test for this. Therefore, we went with browser test here https://codereview.chromium.org/1692773002/. ==========
https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp (right): https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:8: #include "public/platform/WebTaskRunner.h" On 2016/03/01 01:08:56, hiroshige wrote: > nit: probably can we remove #include for Platform.h and WebTaskRunner.h? Done. https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:15: , m_frameInterval(1 / frameRate) On 2016/02/29 17:39:49, philipj_UTC7 wrote: > On 2016/02/29 17:00:53, mcasas wrote: > > nit: perhaps unnecessary, but I suggest > > s/1/1.0/ so it's super clear that those are > > floating point values. > > https://www.chromium.org/blink/coding-style#TOC-Floating-point-literals says to > only use it to force floating point math. I can't say it's obvious to me that it > isn't needed here, but if it is, then the existing code would be very broken as > m_frameInterval would be zero. It works currently because |frameRate| is double, as mcasas@ mentioned. https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:15: , m_frameInterval(1 / frameRate) On 2016/02/29 17:43:17, mcasas wrote: > On 2016/02/29 17:39:49, philipj_UTC7 wrote: > > On 2016/02/29 17:00:53, mcasas wrote: > > > nit: perhaps unnecessary, but I suggest > > > s/1/1.0/ so it's super clear that those are > > > floating point values. > > > > https://www.chromium.org/blink/coding-style#TOC-Floating-point-literals says > to > > only use it to force floating point math. I can't say it's obvious to me that > it > > isn't needed here, but if it is, then the existing code would be very broken > as > > m_frameInterval would be zero. > > To clarify: it isn't needed (the code is correct w |1| since > |frameRate| is double), I just find it easier to read :) > (kind of self documenting if you want). I will keep it |1| to comply to the coding style. https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:26: listener->m_requestFrameTimer.startRepeating(listener->m_frameInterval, BLINK_FROM_HERE); On 2016/02/29 03:51:25, philipj_UTC7 wrote: > Yeah, judging by TimerBase::runInternal this should do the trick when it comes > to drift, but it actually won't solve the problem of frame drops due to time > jitter. It does solve the memory leak, though. Yes, I will update the CL description to make it clear. https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:38: m_frameCaptureRequested = true; On 2016/02/29 17:00:53, mcasas wrote: > nit: Perhaps something for down the line, but > you could add some dev provision to check > for how much deviation/jitter we're actually > getting versus expected, and maybe add > some logging of sorts if, say, it deviates > 10% ? > I'm happy with a crbug too, if any. I am adding a TODO referencing to the current bug. In the other test, we added a tolerance but it also involves the video playback code which might include additional jitter and drift. [0] https://codereview.chromium.org/1692773002/ https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.h (right): https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.h:17: On 2016/02/29 03:51:25, philipj_UTC7 wrote: > Did you intentionally add this blank line? You didn't add one before private: > below. Removed, it was a clang autoformat tool artifact. https://codereview.chromium.org/1746443002/diff/10001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.h:26: void requestFrameTimerFired(Timer<TimedCanvasDrawListener>*); On 2016/02/29 17:00:53, mcasas wrote: > nit: Can you add a comment and say this complies to > a TimerFiredFunction signature? Done.
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, mcasas@chromium.org, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1746443002/#ps30001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1746443002/30001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1746443002/30001
Message was sent while issue was closed.
Description was changed from ========== Use Timer in TimedCanvasDrawListener In the initial implementation, we used postDelayedTask() for repeating tasks in TimedCanvasDrawListener. However, this would introduce drift, jitter and a possible memory leak if tasks in flight prevent the class from being GC'ed. This CL uses UnthrottledTimer instead for the same purposes, which addresses drift and memory leak issues. BUG=589974 TEST=It is very difficult to write a non-flaky Layout Test for this. Therefore, we went with browser test here https://codereview.chromium.org/1692773002/. ========== to ========== Use Timer in TimedCanvasDrawListener In the initial implementation, we used postDelayedTask() for repeating tasks in TimedCanvasDrawListener. However, this would introduce drift, jitter and a possible memory leak if tasks in flight prevent the class from being GC'ed. This CL uses UnthrottledTimer instead for the same purposes, which addresses drift and memory leak issues. BUG=589974 TEST=It is very difficult to write a non-flaky Layout Test for this. Therefore, we went with browser test here https://codereview.chromium.org/1692773002/. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:30001)
Message was sent while issue was closed.
Description was changed from ========== Use Timer in TimedCanvasDrawListener In the initial implementation, we used postDelayedTask() for repeating tasks in TimedCanvasDrawListener. However, this would introduce drift, jitter and a possible memory leak if tasks in flight prevent the class from being GC'ed. This CL uses UnthrottledTimer instead for the same purposes, which addresses drift and memory leak issues. BUG=589974 TEST=It is very difficult to write a non-flaky Layout Test for this. Therefore, we went with browser test here https://codereview.chromium.org/1692773002/. ========== to ========== Use Timer in TimedCanvasDrawListener In the initial implementation, we used postDelayedTask() for repeating tasks in TimedCanvasDrawListener. However, this would introduce drift, jitter and a possible memory leak if tasks in flight prevent the class from being GC'ed. This CL uses UnthrottledTimer instead for the same purposes, which addresses drift and memory leak issues. BUG=589974 TEST=It is very difficult to write a non-flaky Layout Test for this. Therefore, we went with browser test here https://codereview.chromium.org/1692773002/. Committed: https://crrev.com/91f06f9ef435b692f4ed020c2cb619f7e2201848 Cr-Commit-Position: refs/heads/master@{#378616} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/91f06f9ef435b692f4ed020c2cb619f7e2201848 Cr-Commit-Position: refs/heads/master@{#378616} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
