Description was changed from
==========
timed
BUG=
==========
to
==========
Add TimedCanvasDrawListener
This CL handles captureStream(frameRate) case where we are expected
to capture frames from a canvas with respect to the given frame rate
constraints. It adds a new CanvasDrawListener, and refactors the existing
interface.
BUG=254990
TEST= Ran the demo with MediaCaptureFromElement flag enabled.
https://rawgit.com/uysalere/js-demos/master/canvascapture4.html
==========
Description was changed from
==========
Add TimedCanvasDrawListener
This CL handles captureStream(frameRate) case where we are expected
to capture frames from a canvas with respect to the given frame rate
constraints. It adds a new CanvasDrawListener, and refactors the existing
interface.
BUG=254990
TEST= Ran the demo with MediaCaptureFromElement flag enabled.
https://rawgit.com/uysalere/js-demos/master/canvascapture4.html
==========
to
==========
Add TimedCanvasDrawListener
This CL handles captureStream(frameRate) case where we are expected
to capture frames from a canvas with respect to the given frame rate
constraints. It adds a new CanvasDrawListener, and refactors the existing
interface.
BUG=524218
TEST= Ran the demo with MediaCaptureFromElement flag enabled.
https://rawgit.com/uysalere/js-demos/master/canvascapture4.html
==========
esprehn
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.cpp File third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.cpp (right): https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.cpp#newcode24 third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.cpp:24: m_handler = handler; why not in the initializer list? ...
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.cpp (right):
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.cpp:24: m_handler
= handler;
On 2015/12/11 10:38:23, esprehn wrote:
> why not in the initializer list?
>
> : m_handler(handler)
> {
> }
>
> your destructor should probably also be added and out of line
Done.
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h (right):
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h:23:
CanvasDrawListener(const PassOwnPtr<WebCanvasCaptureHandler>&);
On 2015/12/11 10:38:23, esprehn wrote:
> explicit, single argument constructors always need that, even if they're not
> public. Otherwise you might create one by mistake somewhere inside your
class...
Done.
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.cpp
(right):
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.cpp:19:
:CanvasDrawListener(handler)
On 2015/12/11 10:38:23, esprehn wrote:
> missing space
Done.
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.cpp
(right):
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.cpp:16:
CanvasCaptureMediaStreamTrack*
CanvasCaptureMediaStreamTrack::create(MediaStreamComponent* component,
PassRefPtrWillBeRawPtr<HTMLCanvasElement> element, const
PassOwnPtr<WebCanvasCaptureHandler> handler, bool givenframeRate, double
frameRate)
On 2015/12/11 10:38:23, esprehn wrote:
> ditto
Done.
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.cpp:18:
return new CanvasCaptureMediaStreamTrack(component, element, handler,
givenframeRate, frameRate);
On 2015/12/11 10:38:23, esprehn wrote:
> givenFrameRate
Done.
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.cpp:39:
CanvasCaptureMediaStreamTrack::CanvasCaptureMediaStreamTrack(MediaStreamComponent*
component, PassRefPtrWillBeRawPtr<HTMLCanvasElement> element, const
PassOwnPtr<WebCanvasCaptureHandler> handler, bool givenframeRate, double
frameRate)
On 2015/12/11 10:38:23, esprehn wrote:
> givenFrameRate, need to match the frameRate naming
Done.
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.h
(right):
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.h:21:
static CanvasCaptureMediaStreamTrack* create(MediaStreamComponent*,
PassRefPtrWillBeRawPtr<HTMLCanvasElement>, const
PassOwnPtr<WebCanvasCaptureHandler>, bool , double);
On 2015/12/11 10:38:24, esprehn wrote:
> remove space before , also add the argument names.
presubmit gives an error:
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.h:20:
The parameter name "component" adds no information, so it should be removed.
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.h:27:
CanvasCaptureMediaStreamTrack(MediaStreamComponent*,
PassRefPtrWillBeRawPtr<HTMLCanvasElement>, const
PassOwnPtr<WebCanvasCaptureHandler>, bool , double);
On 2015/12/11 10:38:24, esprehn wrote:
> ditto
Done.
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.h
(right):
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.h:23:
static MediaStream* captureStream(HTMLCanvasElement&, bool givenframeRate,
double frameRate, ExceptionState&);
On 2015/12/11 10:38:24, esprehn wrote:
> givenFrameRate
Done.
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp
(right):
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:25:
return CanvasDrawListener::needsNewFrame();
On 2015/12/11 10:38:24, esprehn wrote:
> return m_requestFrame && CanvasDrawListener::needsNewFrame();
Done.
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:33:
Platform::current()->currentThread()->taskRunner()->postDelayedTask(BLINK_FROM_HERE,
new Task(bind(&TimedCanvasDrawListener::requestNewFrame, this)),
m_frameInterval);
On 2015/12/11 10:38:24, esprehn wrote:
> I have to admit this feels suuuper weird. It means we randomly sample the
canvas
> in a task at the frame interval. I think the spec actually should tell us to
> pick the nearest frame boundary that's after that time, so the requestNewFrame
> thing should actually do:
>
> setNeedsBeginFrame()
>
> and at the end of the BeginFrame work we should sample the canvas. That gives
> you predictable timing for when the canvas is sampled.
>
> This is okay to land for now, but we should probably fix the spec.
I totally see your point. I am going to explain it to hta@ to bring it up in the
spec discussion.
For now, I am changing it such that we are listening every interval. This should
work similar to what you explained.
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:38:
:CanvasDrawListener(handler)
On 2015/12/11 10:38:24, esprehn wrote:
> missing space, also lets move this to the top near create
Done.
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.h
(right):
https://codereview.chromium.org/1508023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.h:18:
~TimedCanvasDrawListener() {}
On 2015/12/11 10:38:24, esprehn wrote:
> out of line
Done.
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h (right):
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h:16: class
CORE_EXPORT CanvasDrawListener : public GarbageCollectedMixin {
On 2015/12/15 23:58:01, esprehn wrote:
> I wonder if this should be mixin if it's going to have members?
It looks like Mixin is allowed to have members from the example given [0].
Either way it is an OwnPtr to a chrome object and no tracing is needed right?
[0]
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.cpp
(right):
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.cpp:56:
CanvasCaptureMediaStreamTrack::CanvasCaptureMediaStreamTrack(MediaStreamComponent*
component, PassRefPtrWillBeRawPtr<HTMLCanvasElement> element, const
PassOwnPtr<WebCanvasCaptureHandler> handler, bool givenFrameRate, double
frameRate)
On 2015/12/15 23:58:01, esprehn wrote:
> this is very strange, lets just add two constructors, one with a frameFrame,
and
> one without. Having the bool where it controls if we respect or ignore that
> other arg is weird.
Done.
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.h
(right):
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.h:20:
static CanvasCaptureMediaStreamTrack* create(MediaStreamComponent*,
PassRefPtrWillBeRawPtr<HTMLCanvasElement>, const
PassOwnPtr<WebCanvasCaptureHandler>, bool, double);
On 2015/12/15 23:58:01, esprehn wrote:
> Sorry I wasn't clear, the rule is all primitive types need the names. So the
> bool and double need the names, not the handler or the canvas or the
component.
>
> The rule is: "Can I tell what this variable is for just from the type".
>
> which is almost always false for primitives
Thanks, it makes sense now :)
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStreamTrack.h:31:
CanvasCaptureMediaStreamTrack(MediaStreamComponent*,
PassRefPtrWillBeRawPtr<HTMLCanvasElement>, const
PassOwnPtr<WebCanvasCaptureHandler>, bool, double);
On 2015/12/15 23:58:01, esprehn wrote:
> add bool and double names
Done.
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp
(right):
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp:25:
return HTMLCanvasElementCapture::captureStream(element, false,
kDefaultFrameRate, exceptionState);
On 2015/12/15 23:58:01, esprehn wrote:
> Hmmm that bool seems like it should be an enum, it's not clear from here what
> the true vs false does.
>
> Actually lets just add two separate methods, one that takes a frameRate and
one
> that doesn't.
I am adding "if (givenFrameRate)" cases to avoid code duplication. I explained
the use of kDefaultFrameRate below.
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp:42:
OwnPtr<WebCanvasCaptureHandler> handler =
adoptPtr(Platform::current()->createCanvasCaptureHandler(size, frameRate,
&track));
On 2015/12/15 23:58:01, esprehn wrote:
> you pass frameRate here even if givenFrameRate is false, how does it know? It
> seems like you can just use a magic number for frameRate instead if its okay
to
> do it here.
This is some confusing part, more related to VideoCapturerSource than this spec.
captureStream() case doesn't have a frame rate, but VideoCapturerSource
implementation requires to return a frame rate [0]. <canvas> doesn't have a
frame rate and when it is not specified here, I return a default value.
[0]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp:50:
tracks.append(CanvasCaptureMediaStreamTrack::create(track, &element,
handler.release(), givenFrameRate, frameRate));
On 2015/12/15 23:58:01, esprehn wrote:
> Split this method into two methods, one for frameRate and one without.
Done.
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.h
(right):
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.h:21:
static MediaStream* captureStream(HTMLCanvasElement&, double frameRate,
ExceptionState&);
On 2015/12/15 23:58:01, esprehn wrote:
> ditto
Done.
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.h:23:
static MediaStream* captureStream(HTMLCanvasElement&, bool givenFrameRate,
double frameRate, ExceptionState&);
On 2015/12/15 23:58:01, esprehn wrote:
> ditto
Done.
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp
(right):
https://codereview.chromium.org/1508023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/TimedCanvasDrawListener.cpp:20:
requestNewFrame();
On 2015/12/15 23:58:01, esprehn wrote:
> can we do this in the caller? The constructor having side effects is a little
> sad.
Done.
esprehn
I'd really rather you split the code up so you didn't need need this givenFrameRate ...
Also where are your layout tests? :) This definitely needs a bunch of tests!
emircan
On 2015/12/19 03:33:31, esprehn wrote: > Also where are your layout tests? :) This definitely ...
4 years, 11 months ago
(2015-12-29 22:57:09 UTC)
#15
On 2015/12/19 03:33:31, esprehn wrote:
> Also where are your layout tests? :) This definitely needs a bunch of tests!
I worked on writing a layout tests where I try to count the number of incoming
frames per second. However, what I found is that there aren't enough JS events
to do the job. MediaStream and <video> do not provide any event notifying the
new frame, only the beginning and end of stream. I wrote a test with JS
setTimeout() which turned out to be flaky. The advice I got is to write a new JS
event behind a blink test flag such as onNewCaptureStreamFrame. I will discuss
this further when people are back from holidays and definitely revisit, see
https://code.google.com/p/chromium/issues/detail?id=565215.
https://codereview.chromium.org/1508023002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h (right):
https://codereview.chromium.org/1508023002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h:23: explicit
CanvasDrawListener(const PassOwnPtr<WebCanvasCaptureHandler>&);
On 2015/12/19 03:33:04, esprehn wrote:
> PassOwnPtr should be by value, never by reference
Done.
https://codereview.chromium.org/1508023002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp
(right):
https://codereview.chromium.org/1508023002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp:57:
tracks.append(CanvasCaptureMediaStreamTrack::create(track, &element,
handler.release()));
On 2015/12/19 03:33:04, esprehn wrote:
> I don't understand why we can't use kDefaultFrameRate here?
I need a way to distinguish between captureStream(60) and captureStream() calls.
captureStream(60) should lead to TimedCanvasDrawListener and captureStream()
should lead to AutoCanvasDrawListener. kDefaultFrameRate has nothing to do with
blink side classes or CanvasDrawListeners.
It is just that the Chrome side VideoCapturerSource interface[0] needs to return
some frame_rate information. I decided to have a single
Platform::current()->createCanvasCaptureHandler() signature and define the
kDefaultFrameRate here, see l.46. I can revisit this later as each
captureStream() mode is introduced.
[0]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_c...
emircan
The CQ bit was checked by emircan@chromium.org
4 years, 11 months ago
(2015-12-29 23:42:41 UTC)
#16
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508023002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508023002/140001
4 years, 11 months ago
(2015-12-29 23:42:49 UTC)
#18
Description was changed from ========== Add TimedCanvasDrawListener This CL handles captureStream(frameRate) case where we are ...
4 years, 11 months ago
(2015-12-29 23:47:50 UTC)
#19
Message was sent while issue was closed.
Description was changed from
==========
Add TimedCanvasDrawListener
This CL handles captureStream(frameRate) case where we are expected
to capture frames from a canvas with respect to the given frame rate
constraints. It adds a new CanvasDrawListener, and refactors the existing
interface.
BUG=524218
TEST= Ran the demo with MediaCaptureFromElement flag enabled.
https://rawgit.com/uysalere/js-demos/master/canvascapture4.html
==========
to
==========
Add TimedCanvasDrawListener
This CL handles captureStream(frameRate) case where we are expected
to capture frames from a canvas with respect to the given frame rate
constraints. It adds a new CanvasDrawListener, and refactors the existing
interface.
BUG=524218
TEST= Ran the demo with MediaCaptureFromElement flag enabled.
https://rawgit.com/uysalere/js-demos/master/canvascapture4.html
==========
commit-bot: I haz the power
Committed patchset #5 (id:140001)
4 years, 11 months ago
(2015-12-29 23:47:51 UTC)
#20
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
commit-bot: I haz the power
Description was changed from ========== Add TimedCanvasDrawListener This CL handles captureStream(frameRate) case where we are ...
4 years, 11 months ago
(2015-12-29 23:48:49 UTC)
#21
Message was sent while issue was closed.
Description was changed from
==========
Add TimedCanvasDrawListener
This CL handles captureStream(frameRate) case where we are expected
to capture frames from a canvas with respect to the given frame rate
constraints. It adds a new CanvasDrawListener, and refactors the existing
interface.
BUG=524218
TEST= Ran the demo with MediaCaptureFromElement flag enabled.
https://rawgit.com/uysalere/js-demos/master/canvascapture4.html
==========
to
==========
Add TimedCanvasDrawListener
This CL handles captureStream(frameRate) case where we are expected
to capture frames from a canvas with respect to the given frame rate
constraints. It adds a new CanvasDrawListener, and refactors the existing
interface.
BUG=524218
TEST= Ran the demo with MediaCaptureFromElement flag enabled.
https://rawgit.com/uysalere/js-demos/master/canvascapture4.html
Committed: https://crrev.com/1d8dcb8c58902f2493c4ff116e8696168a2986a2
Cr-Commit-Position: refs/heads/master@{#367118}
==========
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/1d8dcb8c58902f2493c4ff116e8696168a2986a2 Cr-Commit-Position: refs/heads/master@{#367118}
4 years, 11 months ago
(2015-12-29 23:48:52 UTC)
#22
Issue 1508023002: Add TimedCanvasDrawListener
(Closed)
Created 5 years ago by emircan
Modified 4 years, 11 months ago
Reviewers: esprehn
Base URL: https://chromium.googlesource.com/chromium/src.git@spec-change
Comments: 50
Chromium Code Reviews
Issue
1508023002:
Add TimedCanvasDrawListener (Closed)
