Basic use implementation for MediaStream from Canvas: captureStream()

Basic use implementation for MediaStream from Canvas: captureStream()

This CL handles captureStream() case where frame rate is omitted and
"the user agent should capture new frames each time that the content
of the canvas changes". HTMLCanvasElement "pushes" new frames to
AutoCanvasDrawListener when it is ready.

Please look at the detailed updated design doc here: https://goo.gl/BFJY52
BUG=254990
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0] https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t78/edit#heading=h.gee451fatrq7

Committed: https://crrev.com/15d1bb360b49621c833febd823b68954de99802b
Cr-Commit-Position: refs/heads/master@{#363451}

[emircan, 2015-11-23 21:20:29]

Patchset #1 (id:1) has been deleted

[emircan, 2015-11-23 21:21:52]

Description was changed from

==========
new interfaces

BUG=
==========

to

==========
Interfaces for MediaStream from Canvas

TO Be FILLED
==========

[emircan, 2015-11-23 21:21:52]

emircan@chromium.org changed reviewers:
+ esprehn@chromium.org

[emircan, 2015-11-23 23:23:11]

Patchset #1 (id:20001) has been deleted

[emircan, 2015-11-24 00:27:04]

Patchset #1 (id:40001) has been deleted

[emircan, 2015-11-24 02:39:51]

Patchset #1 (id:60001) has been deleted

[emircan, 2015-11-24 02:41:15]

Description was changed from

==========
Interfaces for MediaStream from Canvas

TO Be FILLED
==========

to

==========
Basic use implementation for MediaStream from Canvas: captureStream()

Please look at the detailed design doc here: https://goo.gl/BFJY52 

BUG=254990
TEST= Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

[emircan, 2015-11-24 03:10:27]

Description was changed from

==========
Basic use implementation for MediaStream from Canvas: captureStream()

Please look at the detailed design doc here: https://goo.gl/BFJY52 

BUG=254990
TEST= Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

to

==========
Basic use implementation for MediaStream from Canvas: captureStream()

This CL handles captureStream() case where frame rate is omitted and "the user
agent should capture new frames each time that the content of the canvas
changes". HTMLCanvasElement "pushed" new frames to AutoCanvasDrawListener when
it is ready. 
 
Please look at the detailed updated design doc here: https://goo.gl/BFJY52 
BUG=254990
TEST= Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

[emircan, 2015-11-24 03:14:58]

emircan@chromium.org changed reviewers:
+ junov@google.com, kbr@chromium.org, keishi@chromium.org, peria@chromium.org,
perkj@chromium.org

[emircan, 2015-11-24 03:14:59]

Please take a look.

[emircan, 2015-11-24 03:15:25]

emircan@chromium.org changed reviewers:
+ junov@chromium.org
- junov@google.com

[peria, 2015-11-24 05:50:22]

I put some comments around Oilpan infrastructure.

To learn about Oilpan, please read this presentation
https://docs.google.com/presentation/d/1XPu03ymz8W295mCftEC9KshH9Icxfq81YwIJQ...
and this document (on being updated)
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:206:
WillBeHeapHashSet<RawPtrWillBeMember<CanvasDrawListener>> m_listeners;
Can this type be
PersistentHeapHashSetWillBeHeapHashSet<WeakMember<CanvasDrawListener>>
?

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h (right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h:15: class
CORE_EXPORT CanvasDrawListener : public WillBeGarbageCollectedMixin {
s/WillBe//.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.cpp
(right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.cpp:15:
return adoptPtrWillBeNoop(new AutoCanvasDrawListener(handler));
return new AutoCanvasDrawListener(handler);

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.h
(right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.h:15:
class AutoCanvasDrawListener final : public
NoBaseWillBeGarbageCollectedFinalized<AutoCanvasDrawListener>, public
CanvasDrawListener {
s/NoBaseWillbeG//.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.h:16:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(AutoCanvasDrawListener);
s/WILL_BE_//.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.h:19:
static PassOwnPtrWillBeRawPtr<AutoCanvasDrawListener> create(const
PassOwnPtr<WebCanvasCaptureHandler>&);
AutoCanvasDrawListener* crete(...);

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.cpp
(right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.cpp:19:
m_canvasElement->removeListener(m_drawListener.get());
Remove this line.

As a basic rule, we cannot touch Member pointers in destructors.
And if m_drawListener dies with this CanvasCaptureMediaStream,
it will be unregistered from m_canvasElement->m_listeners in Oilpan's
weak processing.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h
(right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:23:
static CanvasCaptureMediaStream* create(MediaStreamDescriptor*,
HTMLCanvasElement&, const PassOwnPtr<WebCanvasCaptureHandler>&);
It is not recommended to pass an argument by non-const reference.  Please use a
raw pointer or PassRefPtrWillBeRawPtr<HTMLCanvasElement> instead.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:23:
static CanvasCaptureMediaStream* create(MediaStreamDescriptor*,
HTMLCanvasElement&, const PassOwnPtr<WebCanvasCaptureHandler>&);
nit: PassOwnPtr<> does not require &.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:29:
CanvasCaptureMediaStream(MediaStreamDescriptor*, HTMLCanvasElement&, const
PassOwnPtr<WebCanvasCaptureHandler>&);
ditto.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:30:
RawPtrWillBeMember<HTMLCanvasElement> m_canvasElement;
I'm not sure how m_canvasElement's lifetime is managed, but shouldn't this be
RefPtrWillBeMember<>?

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:31:
OwnPtrWillBeMember<CanvasDrawListener> m_drawListener;
s/OwnPtrWillBe//.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp
(right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp:21:
CanvasCaptureMediaStream*
HTMLCanvasElementCapture::captureStream(HTMLCanvasElement& element)
Please do not use non-const reference.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.h
(right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.h:18:
static CanvasCaptureMediaStream* captureStream(HTMLCanvasElement&);
Please use a raw pointer or PassRefPtrWillBeRawPtr<HTMLCanvasElement> for this
argument.

[emircan, 2015-11-24 22:14:48]

Patchset #2 (id:100001) has been deleted

[emircan, 2015-11-24 22:16:52]

Thanks peria@. I wrongly assumed CanvasDrawListener* would be WillBe* as it is
following HTMLCanvasElement, but I will clean those up now.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:206:
WillBeHeapHashSet<RawPtrWillBeMember<CanvasDrawListener>> m_listeners;
On 2015/11/24 05:50:22, peria wrote:
> Can this type be
> PersistentHeapHashSetWillBeHeapHashSet<WeakMember<CanvasDrawListener>>
> ?

Done.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h (right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h:15: class
CORE_EXPORT CanvasDrawListener : public WillBeGarbageCollectedMixin {
On 2015/11/24 05:50:22, peria wrote:
> s/WillBe//.

Done.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.cpp
(right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.cpp:15:
return adoptPtrWillBeNoop(new AutoCanvasDrawListener(handler));
On 2015/11/24 05:50:22, peria wrote:
> return new AutoCanvasDrawListener(handler);

Done.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.h
(right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.h:15:
class AutoCanvasDrawListener final : public
NoBaseWillBeGarbageCollectedFinalized<AutoCanvasDrawListener>, public
CanvasDrawListener {
On 2015/11/24 05:50:22, peria wrote:
> s/NoBaseWillbeG//.

Done.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.h:16:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(AutoCanvasDrawListener);
On 2015/11/24 05:50:22, peria wrote:
> s/WILL_BE_//.

Done.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.h:19:
static PassOwnPtrWillBeRawPtr<AutoCanvasDrawListener> create(const
PassOwnPtr<WebCanvasCaptureHandler>&);
On 2015/11/24 05:50:22, peria wrote:
> AutoCanvasDrawListener* crete(...);

Done.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.cpp
(right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.cpp:19:
m_canvasElement->removeListener(m_drawListener.get());
On 2015/11/24 05:50:22, peria wrote:
> Remove this line.
> 
> As a basic rule, we cannot touch Member pointers in destructors.
> And if m_drawListener dies with this CanvasCaptureMediaStream,
> it will be unregistered from m_canvasElement->m_listeners in Oilpan's
> weak processing.

Done.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h
(right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:23:
static CanvasCaptureMediaStream* create(MediaStreamDescriptor*,
HTMLCanvasElement&, const PassOwnPtr<WebCanvasCaptureHandler>&);
On 2015/11/24 05:50:22, peria wrote:
> It is not recommended to pass an argument by non-const reference.  Please use
a
> raw pointer or PassRefPtrWillBeRawPtr<HTMLCanvasElement> instead.

Done but there are problems. See the answer below.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:29:
CanvasCaptureMediaStream(MediaStreamDescriptor*, HTMLCanvasElement&, const
PassOwnPtr<WebCanvasCaptureHandler>&);
On 2015/11/24 05:50:22, peria wrote:
> ditto.

Done.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:30:
RawPtrWillBeMember<HTMLCanvasElement> m_canvasElement;
On 2015/11/24 05:50:22, peria wrote:
> I'm not sure how m_canvasElement's lifetime is managed, but shouldn't this be
> RefPtrWillBeMember<>?

RefPtrWillBeMember makes sense to me as well and I made the change. However, I
get the following error during destruction in this way. Is it proper to use
adoptRef() in HTMLCanvasElementCapture::captureStream() to create reference to
the class. Do you see any other issue?

https://paste.googleplex.com/6030890702471168

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:31:
OwnPtrWillBeMember<CanvasDrawListener> m_drawListener;
On 2015/11/24 05:50:22, peria wrote:
> s/OwnPtrWillBe//.

Done.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp
(right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp:21:
CanvasCaptureMediaStream*
HTMLCanvasElementCapture::captureStream(HTMLCanvasElement& element)
On 2015/11/24 05:50:22, peria wrote:
> Please do not use non-const reference.

Same as the answer above.

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.h
(right):

https://codereview.chromium.org/1467103003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.h:18:
static CanvasCaptureMediaStream* captureStream(HTMLCanvasElement&);
On 2015/11/24 05:50:22, peria wrote:
> Please use a raw pointer or PassRefPtrWillBeRawPtr<HTMLCanvasElement> for this
> argument.

This class is a partial interface of HTMLCanvasElement defined by the spec. So
it isn't really a parameter here, just a reference to the class. As far as I saw
from the V8 examples, this is the proper use?

[esprehn, 2015-11-24 22:24:19]

Non const refs are fine in blink code, they make it clear you don't expect
null. We use them all over the place. Assigning to a Node fails anyway so
there's no danger.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2015-11-24 22:24:19]

Non const refs are fine in blink code, they make it clear you don't expect
null. We use them all over the place. Assigning to a Node fails anyway so
there's no danger.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Justin Novosad, 2015-11-25 02:38:05]

Needs layout tests.

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:408: ASSERT(status ==
NormalSourceImageStatus);
Are you sure about this? I think you should be doing some error handling here.
For example, what if the size of the canvas gets set to zero?

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:409:
listener->setNewFrameCapture(sourceImage->imageForCurrentFrame());
Before sending the new frame, you need to check the origin-clean flag and mute
the track origin-clean is false.

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:206:
PersistentHeapHashSetWillBeHeapHashSet<WeakMember<CanvasDrawListener>>
m_listeners;
I feel bad that you had to figure out how to declare this. That being said, I
believe you neglected to add this frankenmember to the trace method. I just
kicked off an oilpan try job to see.

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h
(right):

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:29:
RefPtrWillBeMember<HTMLCanvasElement> m_canvasElement;
This strong ref implies that the canvas, along with its rendering context, will
be kept alive by CanvasCaptureMediaStream. The spec does not specify this
behavior. I don't think it is necessarily bad to hold a strong ref, but I think
it needs to be clarified in the spec.

[emircan, 2015-11-26 01:02:00]

Patchset #4 (id:160001) has been deleted

[emircan, 2015-11-26 01:03:52]

I am starting to work on layout tests.

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:408: ASSERT(status ==
NormalSourceImageStatus);
On 2015/11/25 02:38:05, Justin Novosad wrote:
> Are you sure about this? I think you should be doing some error handling here.
> For example, what if the size of the canvas gets set to zero?

Are you refering to ZeroSizeCanvasSourceImageStatus case? Would (status !=
NormalSourceImageStatus) be enough here or I should check other cases as well?

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:409:
listener->setNewFrameCapture(sourceImage->imageForCurrentFrame());
On 2015/11/25 02:38:05, Justin Novosad wrote:
> Before sending the new frame, you need to check the origin-clean flag and mute
> the track origin-clean is false.

I was planning to leave those checks for later as this CL is growing. I am
adding a check here and one in captureStream () method.

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:206:
PersistentHeapHashSetWillBeHeapHashSet<WeakMember<CanvasDrawListener>>
m_listeners;
On 2015/11/25 02:38:05, Justin Novosad wrote:
> I feel bad that you had to figure out how to declare this. That being said, I
> believe you neglected to add this frankenmember to the trace method. I just
> kicked off an oilpan try job to see.

Right, thanks for catching. I am adding.

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h
(right):

https://codereview.chromium.org/1467103003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:29:
RefPtrWillBeMember<HTMLCanvasElement> m_canvasElement;
On 2015/11/25 02:38:05, Justin Novosad wrote:
> This strong ref implies that the canvas, along with its rendering context,
will
> be kept alive by CanvasCaptureMediaStream. The spec does not specify this
> behavior. I don't think it is necessarily bad to hold a strong ref, but I
think
> it needs to be clarified in the spec.

The spec says that HTMLCanvasElement is a readonly attribute. Would it be ok to
return nullptr for it? I will try to clear it with hta.

[perkj_chrome, 2015-11-26 18:52:14]

Nice. Sorry I have not looked at this in a while. 
I focused on the Chrome parts. I am not sure I understand the ownership of
CanvasCaptureHandler. It is refcounted but I can only find one ref and that is
from the VideoSource you create.

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:23: canvas_handler)
Have you tried git cl format?

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:92:
content::AddVideoTrackToMediaStream(video.Pass(), false, false, stream);
I think you should mark this as read_only. Ie - you can not reconfigure it using
constraints? 
If that is changed in the future, you can mark it as writable then.

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:101: createNewFrame(image);
add thread checker

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:105: return ask_for_new_frame_;
add thread checker

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:105: return ask_for_new_frame_;
ask_for_new_frame_ this seems to be unnessecary?

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:170:
base::Unretained(delegate_.get()), video_frame,
This does not seem to be safe. You post to the io thread on a scoped_ptr. What
guarantees that CanvasCaptureHandler is kept alive and does not delete the
delegate_ before the task is handled?

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.h (right):

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:27: public
base::RefCounted<CanvasCaptureHandler> {
RefCounted? Does this object really need to be? It seems like the only place
that holds a reference to this object is
CanvasCaptureHandler::VideoCapturerSource that is directly owned by a blink
object.

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:36: //
media::VideoCapturerSource Implementation.
This class does not implement media::VideoCapturerSource .

[Justin Novosad, 2015-11-27 01:38:12]

On 2015/11/26 01:03:52, emircan wrote:

> On 2015/11/25 02:38:05, Justin Novosad wrote:
> > This strong ref implies that the canvas, along with its rendering context,
> will
> > be kept alive by CanvasCaptureMediaStream. The spec does not specify this
> > behavior. I don't think it is necessarily bad to hold a strong ref, but I
> think
> > it needs to be clarified in the spec.
> 
> The spec says that HTMLCanvasElement is a readonly attribute. Would it be ok
to
> return nullptr for it? I will try to clear it with hta.

Exactly, that is the right question to ask, and depending on the answer you will
have a different layout test to write. Assuming that the answer is that you do
indeed need to hold a strong re,f you should have a test that dereferences the
canvas and its rendering context after having set up the stream, force a garbage
collection, then verify that you can re-acquire the rendering context with
myCanvasStream.canvas.getContext(...) and continue rendering to it and capture
still works.

[emircan, 2015-12-01 00:25:45]

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:23: canvas_handler)
On 2015/11/26 18:52:14, perkj wrote:
> Have you tried git cl format?

Fixed it. I am using clang-format:Chromium on Eclipse and it somehow aligns it
like this.

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:92:
content::AddVideoTrackToMediaStream(video.Pass(), false, false, stream);
On 2015/11/26 18:52:14, perkj wrote:
> I think you should mark this as read_only. Ie - you can not reconfigure it
using
> constraints? 
> If that is changed in the future, you can mark it as writable then.

Done.

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:101: createNewFrame(image);
On 2015/11/26 18:52:14, perkj wrote:
> add thread checker

Done.

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:105: return ask_for_new_frame_;
On 2015/11/26 18:52:14, perkj wrote:
> add thread checker

Done.

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:105: return ask_for_new_frame_;
On 2015/11/26 18:52:14, perkj wrote:
> ask_for_new_frame_ this seems to be unnessecary? 

When CanvasCaptureHandler::VideoCapturerSource::StopCapture() is called,
CanvasCaptureHandler::StopVideoCapture() sets this field to false.
HTMLCanvasElement calls AutoCanvasDrawListener::needsNewFrameCapture(); and that
calls Chrome side CanvasCaptureHandler::needsNewFrameCapture() which returns
false.

Between the time StopCapture() is called and CanvasCaptureMediaStream() goes out
of scope, we do not receive frames.

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:170:
base::Unretained(delegate_.get()), video_frame,
On 2015/11/26 18:52:14, perkj wrote:
> This does not seem to be safe. You post to the io thread on a scoped_ptr. What
> guarantees that CanvasCaptureHandler is kept alive and does not delete the
> delegate_ before the task is handled? 

Thanks for pointing out. Added a weak_ptr for the delegate.

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.h (right):

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:27: public
base::RefCounted<CanvasCaptureHandler> {
On 2015/11/26 18:52:14, perkj wrote:
> RefCounted? Does this object really need to be? It seems like the only place
> that holds a reference to this object is
> CanvasCaptureHandler::VideoCapturerSource that is directly owned by a blink
> object. 

Here is my logic. I am not sure if I am doing the right thing, but I am
following CastReceiverSession::VideoCapturerSource design here.

::VideoCapturerSource is owned by MediaStreamSource in blink side.
CanvasCaptureHandler is owned by CanvasDrawListener which is owned by
CanvasCaptureMediaStream in blink side. 

I want to forward the calls to ::VideoCapturerSource to CanvasCaptureHandler so
I need a reference between. My assumption is that CanvasCaptureHandler is alive
during the lifetime of ::VideoCapturerSource because the MediaStreamSource goes
out of scope before CanvasCaptureMediaStream does. 

I am trying to have a reference to CanvasCaptureHandler from 2 classes:
CanvasDrawListener on blink side, and ::VideoCapturerSource from Chrome side.
AFAIU, CanvasDrawListener needs to own ptr it receives. ::VideoCapturerSource
cannot have a scoped_ptr<> and a naked pointer didn't look right to me. However,
thinking again, it needs to be a naked ptr as I do not want to call
CanvasCaptureHandler dtor() from Chrome side. So, I am changing it. 

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/me...

https://codereview.chromium.org/1467103003/diff/200001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:36: //
media::VideoCapturerSource Implementation.
On 2015/11/26 18:52:14, perkj wrote:
> This class does not implement media::VideoCapturerSource .

Leftover from earlier. Fixing it.

[emircan, 2015-12-01 00:28:31]

On 2015/11/27 01:38:12, Justin Novosad wrote:
> On 2015/11/26 01:03:52, emircan wrote:
> 
> > On 2015/11/25 02:38:05, Justin Novosad wrote:
> > > This strong ref implies that the canvas, along with its rendering context,
> > will
> > > be kept alive by CanvasCaptureMediaStream. The spec does not specify this
> > > behavior. I don't think it is necessarily bad to hold a strong ref, but I
> > think
> > > it needs to be clarified in the spec.
> > 
> > The spec says that HTMLCanvasElement is a readonly attribute. Would it be ok
> to
> > return nullptr for it? I will try to clear it with hta.
> 
> Exactly, that is the right question to ask, and depending on the answer you
will
> have a different layout test to write. Assuming that the answer is that you do
> indeed need to hold a strong re,f you should have a test that dereferences the
> canvas and its rendering context after having set up the stream, force a
garbage
> collection, then verify that you can re-acquire the rendering context with
> myCanvasStream.canvas.getContext(...) and continue rendering to it and capture
> still works.

hta@ raised the question here:
https://github.com/w3c/mediacapture-fromelement/issues/17 . I think we can
assume a strong ref for this patch, and refer back later.

[emircan, 2015-12-01 00:55:11]

Patchset #6 (id:220001) has been deleted

[emircan, 2015-12-01 19:07:48]

Patchset #7 (id:260001) has been deleted

[perkj_chrome, 2015-12-01 19:26:35]

Keep up the good work!, Changing w3c specs and the world is not easy. Again, I
focused on the chrome parts.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:47: void
CanvasCaptureHandler::VideoCapturerSource::GetCurrentSupportedFormats(
nit: please inline this method too or split out startCapture and StopCapture()

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:65: :
new_frame_callback_(new_frame_callback),
Just use a normal ThreadChecker. In the ctor, call detachThread.

Also, please add a dtor and ad a threadcheck since it looks like it is important
that it this is destroyed on the correct thread since you post to this class on
the io thread.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:85:
DISALLOW_COPY_AND_ASSIGN(CanvasCaptureHandlerDelegate);
nit empty line before DISALLOW...

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:126: delegate_ =
nit: delegate.reset(new ...)
Otherwise you will do a copy of scoped_ptr (unless a there is a move
constructor)

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:137: delegate_.reset();
io_task_runner_->DeleteSoon(FROM ?

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:169: if (!delegate_)
nit: move to the just after the thread_checker_

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.h (right):

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:29: blink::WebMediaStream*
stream);
WebObjects are basically smart pointers so this should probably be const
blink::WebMediaStream&.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:42: blink::WebSize
GetSourceSize() { return size_; }
blink::WebSize (maybe &)  GetSourceSize() const

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:49: // An implementation
VideoCapturerSource that is owned by blink and delegates
implementation of ...

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h
(right):

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:17:
class CanvasCaptureMediaStream final : public MediaStream {
This is going away not that the spec will change?

[esprehn, 2015-12-01 20:04:12]

Description was changed from

==========
Basic use implementation for MediaStream from Canvas: captureStream()

This CL handles captureStream() case where frame rate is omitted and "the user
agent should capture new frames each time that the content of the canvas
changes". HTMLCanvasElement "pushed" new frames to AutoCanvasDrawListener when
it is ready. 
 
Please look at the detailed updated design doc here: https://goo.gl/BFJY52 
BUG=254990
TEST= Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

to

==========
Basic use implementation for MediaStream from Canvas: captureStream()

This CL handles captureStream() case where frame rate is omitted and
"the user agent should capture new frames each time that the content
of the canvas changes". HTMLCanvasElement "pushed" new frames to
AutoCanvasDrawListener when it is ready. 
 
Please look at the detailed updated design doc here: https://goo.gl/BFJY52 
BUG=254990
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

[esprehn, 2015-12-01 20:04:27]

This is looking pretty good.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:169: if (!delegate_)
how do you end up with no delegate here?

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:414: RefPtr<Image>
sourceImage = getSourceImageForCanvas(&status, PreferNoAcceleration);
getSourceImageForCanvas is expensive I think it seems to contain GPU readback
code paths, I don't think you want to do it for each loop iteration. I think you
want to loop through the list first to see if any listeners have
needsNewFrameCapture true, then loop through again doing setNewFrameCapture(...)

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:593:
notifyListenersCanvasChanged();
hmm, this means we do a sync readback the first time we add a listener, that
doesn't seem like a good idea

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:65: class CORE_EXPORT
HTMLCanvasElement : public HTMLElement, public DocumentVisibilityObserver,
public CanvasImageSource, public ImageBufferClient, public ImageBitmapSource {
why did you remove final?

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.cpp
(right):

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.cpp:23:
void AutoCanvasDrawListener::setNewFrameCapture(const WTF::PassRefPtr<SkImage>&
image)
can we make these names match, sendNewFrame() here instead of
setNewFrameCapture?

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.h
(right):

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.h:15:
class AutoCanvasDrawListener final : public
GarbageCollectedFinalized<AutoCanvasDrawListener>, public CanvasDrawListener {
Auto? I'm not sure what that means in this context.

[emircan, 2015-12-01 20:45:36]

Patchset #7 (id:280001) has been deleted

[emircan, 2015-12-01 21:07:34]

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:47: void
CanvasCaptureHandler::VideoCapturerSource::GetCurrentSupportedFormats(
On 2015/12/01 19:26:34, perkj wrote:
> nit: please inline this method too or split out startCapture and StopCapture()

Done.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:65: :
new_frame_callback_(new_frame_callback),
On 2015/12/01 19:26:34, perkj wrote:
> Just use a normal ThreadChecker. In the ctor, call detachThread.
> 
> Also, please add a dtor and ad a threadcheck since it looks like it is
important
> that it this is destroyed on the correct thread since you post to this class
on
> the io thread.

Done.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:85:
DISALLOW_COPY_AND_ASSIGN(CanvasCaptureHandlerDelegate);
On 2015/12/01 19:26:34, perkj wrote:
> nit empty line before DISALLOW...

Done.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:126: delegate_ =
On 2015/12/01 19:26:34, perkj wrote:
> nit: delegate.reset(new ...)
> Otherwise you will do a copy of scoped_ptr (unless a there is a move
> constructor)

Done.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:137: delegate_.reset();
On 2015/12/01 19:26:34, perkj wrote:
> io_task_runner_->DeleteSoon(FROM ?

Done.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:169: if (!delegate_)
On 2015/12/01 19:26:34, perkj wrote:
> nit: move to the just after the thread_checker_

I dont think it necessary to check any more.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:169: if (!delegate_)
On 2015/12/01 20:04:26, esprehn wrote:
> how do you end up with no delegate here?

I was thinking of a possible call to this function after StopVideoCapture(), but
|ask_for_new_frame_| stops it. Removed it.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.h (right):

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:29: blink::WebMediaStream*
stream);
On 2015/12/01 19:26:34, perkj wrote:
> WebObjects are basically smart pointers so this should probably be const
> blink::WebMediaStream&.

MediaStreamApi expects a non-const blink::WebMediaStream* as it modifies and
adds track on it. Here it is considered as an input variable and ptr is more
accurate it looks like. (This would go away after spec change)
 
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:42: blink::WebSize
GetSourceSize() { return size_; }
On 2015/12/01 19:26:34, perkj wrote:
> blink::WebSize (maybe &)  GetSourceSize() const

Done.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:49: // An implementation
VideoCapturerSource that is owned by blink and delegates
On 2015/12/01 19:26:34, perkj wrote:
> implementation of ...

Done.

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:414: RefPtr<Image>
sourceImage = getSourceImageForCanvas(&status, PreferNoAcceleration);
On 2015/12/01 20:04:26, esprehn wrote:
> getSourceImageForCanvas is expensive I think it seems to contain GPU readback
> code paths, I don't think you want to do it for each loop iteration. I think
you
> want to loop through the list first to see if any listeners have
> needsNewFrameCapture true, then loop through again doing
setNewFrameCapture(...)

Done. My bad, I was initially designing it with a single listener in mind.

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:593:
notifyListenersCanvasChanged();
On 2015/12/01 20:04:26, esprehn wrote:
> hmm, this means we do a sync readback the first time we add a listener, that
> doesn't seem like a good idea

I see that frame might not be completed. I was trying to cut down the time on
black frame until next draw happens. I can bring that to the spec discussion.
Removed it.

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:65: class CORE_EXPORT
HTMLCanvasElement : public HTMLElement, public DocumentVisibilityObserver,
public CanvasImageSource, public ImageBufferClient, public ImageBitmapSource {
On 2015/12/01 20:04:26, esprehn wrote:
> why did you remove final?

modules/mediacapturefromelement/HTMLCanvasElementCapture extends it. Initially I
had HTMLCanvasElementCapture as STATIC_ONLY class but couldn't find a way around
adding ExceptionState. Is there a way around that?

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.cpp
(right):

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.cpp:23:
void AutoCanvasDrawListener::setNewFrameCapture(const WTF::PassRefPtr<SkImage>&
image)
On 2015/12/01 20:04:26, esprehn wrote:
> can we make these names match, sendNewFrame() here instead of
> setNewFrameCapture?

Done.

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.h
(right):

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/AutoCanvasDrawListener.h:15:
class AutoCanvasDrawListener final : public
GarbageCollectedFinalized<AutoCanvasDrawListener>, public CanvasDrawListener {
On 2015/12/01 20:04:27, esprehn wrote:
> Auto? I'm not sure what that means in this context.

captureStream(): We listen to each canvas frame change.
captureStream(frameRate): We have a fixed frame rate to respect to.
captureStream(0): We listen only when requested.

I was thinking of AutoCanvasDrawListener, TimedCanvasDrawListener and
OnDemandCanvasDrawListener as the names. WDYT?

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h
(right):

https://codereview.chromium.org/1467103003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:17:
class CanvasCaptureMediaStream final : public MediaStream {
On 2015/12/01 19:26:34, perkj wrote:
> This is going away not that the spec will change?

After offline discussion, we decided to leave it to a smaller followup patch.

[esprehn, 2015-12-01 23:50:29]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org

[esprehn, 2015-12-01 23:50:29]

haraken@ I thought partial interfaces could be implemented using static methods?
I don't think we should make HTMLCanvasElement be non-final for this new
feature, having HTMLCanvasElementCapture be a subclass seems bad, if there's
more than one feature with a partial interface it produces a weird multiple
inheritance mess.

How can emircan@ implement this without removing the final?

[esprehn, 2015-12-01 23:53:53]

Looking at other partial interfaces they're all static methods, are you sure you
need to subclass HTMLCanvasElement, that doesn't seem right.

[haraken, 2015-12-02 00:02:21]

On 2015/12/01 23:53:53, esprehn wrote:
> Looking at other partial interfaces they're all static methods, are you sure
you
> need to subclass HTMLCanvasElement, that doesn't seem right.

You're right. HTMLCanvasElementCapture should not inherit from
HTMLCanvasElement. Given that HTMLCanvasElementCapture is a partial interface of
HTMLCanvasElement, you should get the HTMLCanvasElement via the static methods.

[emircan, 2015-12-02 00:20:47]

Thanks for the help. I changed it to STATIC_ONLY class and it is still fine with
[RaisesException].

[emircan, 2015-12-02 09:07:12]

Patchset #10 (id:360001) has been deleted

[emircan, 2015-12-02 19:05:58]

Patchset #10 (id:380001) has been deleted

[emircan, 2015-12-02 19:08:55]

Patchset #10 (id:400001) has been deleted

[Justin Novosad, 2015-12-02 21:08:16]

https://codereview.chromium.org/1467103003/diff/420001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-output.html
(right):

https://codereview.chromium.org/1467103003/diff/420001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-output.html:31:
for (i = 0; i < 10; i++) {
What is the purpose of this loop?

https://codereview.chromium.org/1467103003/diff/420001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-output.html:48:
video_sink.addEventListener("loadeddata", onCanvasPlaybackStarted, true);
I think the execution order of drawVideoToCanvas vs, onCanvasPlaybackStarted is
non-deterministic. You probably want to test both orders explicitly to: a) have
complete coverage and b) avoid flakiness in the case one of the two orders is
broken.

https://codereview.chromium.org/1467103003/diff/420001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-output.html:51:
setTimeout(onCanvasPlaybackIsOn, 2000);
Arbitrary large delays in layout tests is a no-no.  What exactly is this delay
suppose to achieve?  If it is to give time for something specific to happen,
don't use a delay for that. This is how flaky tests are born. Figure out an
event you can latch on to that will give you the right execution order.

[emircan, 2015-12-03 10:58:21]

Thanks for the review junov@. My goal in that test was to compare the output 
pixel-by-pixel by using dumpAsTextWithPixelResults. I couldn't find a way around

getting frames from mediastream output inside content shell. I decided to 
rewrite the test such that it checks the order of execution in the common use 
case scenario. I added a new bug for this issue blocking the main bug[0]. 

Please take a look again at the 3 layout tests above.
Also, feel free to add comment on the bug[0] regarding the additional layout 
tests you want me to add. 
[0] https://code.google.com/p/chromium/issues/detail?id=565215

[perkj_chrome, 2015-12-03 13:40:21]

content/renderer/media looks good to me with the following nits.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.h (right):

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:29: blink::WebMediaStream*
stream);
On 2015/12/01 21:07:34, emircan wrote:
> On 2015/12/01 19:26:34, perkj wrote:
> > WebObjects are basically smart pointers so this should probably be const
> > blink::WebMediaStream&.
> 
> MediaStreamApi expects a non-const blink::WebMediaStream* as it modifies and
> adds track on it. Here it is considered as an input variable and ptr is more
> accurate it looks like. (This would go away after spec change)
>  
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...

Seems like a mix then. You would typically create your own WebMediaStream based
on the const& version. 
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

https://codereview.chromium.org/1467103003/diff/440001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1467103003/diff/440001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:14: const float
kDefaultFrameRate = 60.0f;
why 60?  Will sendNewFrame be called that frequently per default? Can you
instead have that as a paramter when you create CanvasCaptureHandler since it is
not decided here.

https://codereview.chromium.org/1467103003/diff/440001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.h (right):

https://codereview.chromium.org/1467103003/diff/440001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:11: #include
"base/threading/thread.h"
thread.h not used?

https://codereview.chromium.org/1467103003/diff/440001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:16: #include
"media/base/video_types.h"
not used or used in cc only?

[Justin Novosad, 2015-12-03 15:44:37]

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-creation.html
(right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-creation.html:1:
<!DOCTYPE html>
Missing -expected.txt for this test

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html
(right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html:1:
<!DOCTYPE html>
missing -expected.txt

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html:9:
onStreamActive = test.step_func(function() {
The assertion implementation in testharness.js leaves no traces in the test
results unless it fails. Therefore, this test fails to verify that this event
callback gets called at all.  Same thing for all the other event callbacks in
this test.  There is also no validation of the order of execution of the
callbacks with respect to each other, and with respect to the actions that
trigger them.

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html:56:
test.done();
Maybe I am misunderstanding how this test works, but calling test.done() here
looks like the test will end before any of the event callbacks are called.

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-output.html
(right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-output.html:57:
setTimeout(onCanvasPlaybackIsOn, 2000);
Can we avoid using set timeout with long delays. It is still not clear to me
what this arbitrary delay is for. Please use an actual event instead to
simultaneously avoid flake and long run times. If you want a full paint update
cycle to happen before calling onCanvasPlaybackIsOn, then you should use
testRunner.layoutAndPaintAsyncThen(). If you want to wait for the video to
advance, use the timeupdate event on the video, etc.

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-output.html:67:
setTimeout(drawVideoToCanvas, 100, ctx, video, canvas);
This animation loop should use requestAnimationFrame instead of setTimeout.
Also, does the test really require updates in a continuous, never ending loop?

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-output.html:70:
function onWrite(text){
why the 'on' prefix here?

[emircan, 2015-12-03 18:43:52]

Thanks perkj@. I responded to the nits below.

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.h (right):

https://codereview.chromium.org/1467103003/diff/240001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:29: blink::WebMediaStream*
stream);
On 2015/12/03 13:40:20, perkj wrote:
> On 2015/12/01 21:07:34, emircan wrote:
> > On 2015/12/01 19:26:34, perkj wrote:
> > > WebObjects are basically smart pointers so this should probably be const
> > > blink::WebMediaStream&.
> > 
> > MediaStreamApi expects a non-const blink::WebMediaStream* as it modifies and
> > adds track on it. Here it is considered as an input variable and ptr is more
> > accurate it looks like. (This would go away after spec change)
> >  
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
> 
> Seems like a mix then. You would typically create your own WebMediaStream
based
> on the const& version. 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

I see. Another way I saw is to pass MediaStreamUrl and find inside the
WebMediaStreamRegistry. Either way, this code would go away after the spec
change.

https://codereview.chromium.org/1467103003/diff/440001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1467103003/diff/440001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:14: const float
kDefaultFrameRate = 60.0f;
On 2015/12/03 13:40:20, perkj wrote:
> why 60?  Will sendNewFrame be called that frequently per default? Can you
> instead have that as a paramter when you create CanvasCaptureHandler since it
is
> not decided here.

I was going to add |frame_rate| parameter when I am working on
canvasCapture(double frameRate) cases in the followup patch, but I will do it
now as I am already touching those files.

I am also confused about the frame rate for captureStream() case tbh. We will
get new frames for each canvas content update, so it isn't actually
deterministic from canvas side either. I will bring it up to hta@.

https://codereview.chromium.org/1467103003/diff/440001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.h (right):

https://codereview.chromium.org/1467103003/diff/440001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:11: #include
"base/threading/thread.h"
On 2015/12/03 13:40:20, perkj wrote:
> thread.h not used?

Done.

https://codereview.chromium.org/1467103003/diff/440001/content/renderer/media...
content/renderer/media/canvas_capture_handler.h:16: #include
"media/base/video_types.h"
On 2015/12/03 13:40:21, perkj wrote:
> not used or used in cc only?

Done.

[esprehn, 2015-12-03 18:47:35]

lgtm for the C++ in modules/ with the nits fixed, please fix the layout tests
though.

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-creation.html
(right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-creation.html:1:
<!DOCTYPE html>
On 2015/12/03 at 15:44:37, Justin Novosad wrote:
> Missing -expected.txt for this test

Do web platform tests need a -expected.txt if they just print PASS? I didn't
think they did.

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html
(right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html:56:
test.done();
On 2015/12/03 at 15:44:37, Justin Novosad wrote:
> Maybe I am misunderstanding how this test works, but calling test.done() here
looks like the test will end before any of the event callbacks are called.

Yeah are you sure this is right?

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-output.html
(right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-output.html:3:
<head>
leave out html, head and body

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-output.html:11:
<div id="tx"> Create Real-Time stream from < canvas > and play it back.</div>
&lt; and &gt; don't put spaces like that, remove space at the start

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-output.html:14:
<source src="resources/canvas_video.webm" type='video/webm' />
leave out / on elements

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:412: for
(CanvasDrawListener* listener : m_listeners) {
const ?

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h (right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h:18: virtual bool
needsNewFrameCapture() const = 0;
needsNewFrame()

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebCanvasCaptureHandler.h (right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebCanvasCaptureHandler.h:20: virtual bool
needsNewFrameCapture() const { return false; }
I would name this needsNewFrame() to match sendNewFrame()

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebSkImage.h (right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebSkImage.h:13: template <typename T> class
PassRefPtr;
you could just #include "wtf/Forward.h" inside the INSIDE_BLINK guard

[perkj_chrome, 2015-12-03 19:15:58]

content/renderer/media lgtm with a nit.

https://codereview.chromium.org/1467103003/diff/460001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1467103003/diff/460001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:91: new
CanvasCaptureHandler::VideoCapturerSource(this));
just inject the frame_rate here when you create CanvasCaptureHandler and you
will not need to store it in the handler.

[emircan, 2015-12-03 21:01:52]

Description was changed from

==========
Basic use implementation for MediaStream from Canvas: captureStream()

This CL handles captureStream() case where frame rate is omitted and
"the user agent should capture new frames each time that the content
of the canvas changes". HTMLCanvasElement "pushed" new frames to
AutoCanvasDrawListener when it is ready. 
 
Please look at the detailed updated design doc here: https://goo.gl/BFJY52 
BUG=254990
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

to

==========
Basic use implementation for MediaStream from Canvas: captureStream()

This CL handles captureStream() case where frame rate is omitted and
"the user agent should capture new frames each time that the content
of the canvas changes". HTMLCanvasElement "pushes" new frames to
AutoCanvasDrawListener when it is ready. 
 
Please look at the detailed updated design doc here: https://goo.gl/BFJY52 
BUG=254990
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

[emircan, 2015-12-04 03:23:25]

Thanks for the comments. I addressed your nits below.

I rewrote CanvasCaptureMediaStream-events-and-exceptions.html test based on
MediaStreamTrack example[0]. I track the order of events called. After hooking
up captureStream() to video tag, I draw on the source canvas. Then, I wait on
video.canplaythrough event on the sink and end the tests. I removed all the
setTimeout() calls. Also, I dropped CanvasCaptureMediaStream-output.html test
until I address the bug. 

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-creation.html
(right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-creation.html:1:
<!DOCTYPE html>
On 2015/12/03 15:44:37, Justin Novosad wrote:
> Missing -expected.txt for this test

When I add it, I get the below presubmit warning. I guess, we do not need to add
it when it is expected to pass cleanly.

The following files are passing testharness results without console error
messages, they should be removed:

/work/chromium/src/third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-creation-expected.txt
ERROR: found passing testharness results without console error messages.

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-creation.html:1:
<!DOCTYPE html>
On 2015/12/03 18:47:34, esprehn wrote:
> On 2015/12/03 at 15:44:37, Justin Novosad wrote:
> > Missing -expected.txt for this test
> 
> Do web platform tests need a -expected.txt if they just print PASS? I didn't
> think they did.

Right. "presubmit" checks for that.

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:412: for
(CanvasDrawListener* listener : m_listeners) {
On 2015/12/03 18:47:34, esprehn wrote:
> const ?

Done.

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h (right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasDrawListener.h:18: virtual bool
needsNewFrameCapture() const = 0;
On 2015/12/03 18:47:34, esprehn wrote:
> needsNewFrame()

Done.

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebCanvasCaptureHandler.h (right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebCanvasCaptureHandler.h:20: virtual bool
needsNewFrameCapture() const { return false; }
On 2015/12/03 18:47:34, esprehn wrote:
> I would name this needsNewFrame() to match sendNewFrame()

Done.

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebSkImage.h (right):

https://codereview.chromium.org/1467103003/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebSkImage.h:13: template <typename T> class
PassRefPtr;
On 2015/12/03 18:47:35, esprehn wrote:
> you could just #include "wtf/Forward.h" inside the INSIDE_BLINK guard

Done.

https://codereview.chromium.org/1467103003/diff/460001/content/renderer/media...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1467103003/diff/460001/content/renderer/media...
content/renderer/media/canvas_capture_handler.cc:91: new
CanvasCaptureHandler::VideoCapturerSource(this));
On 2015/12/03 19:15:58, perkj wrote:
> just inject the frame_rate here when you create CanvasCaptureHandler and you
> will not need to store it in the handler.

Done.

[emircan, 2015-12-04 03:28:45]

emircan@chromium.org changed reviewers:
+ sievers@chromium.org

[emircan, 2015-12-04 03:28:46]

sievers@chromium.org: Please review changes in
content/renderer/renderer_blink_platform_impl.*

haraken@chromium.org: Please review changes in
third_party/WebKit/Source/modules/*

[emircan, 2015-12-04 03:37:08]

Patchset #13 (id:480001) has been deleted

[haraken, 2015-12-04 03:55:07]

modules/ LGTM

https://codereview.chromium.org/1467103003/diff/500001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h
(right):

https://codereview.chromium.org/1467103003/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:19:
USING_GARBAGE_COLLECTED_MIXIN(CanvasCaptureMediaStream);

Remove this.

https://codereview.chromium.org/1467103003/diff/500001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp
(right):

https://codereview.chromium.org/1467103003/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp:18:
#include "public/platform/WebMediaStreamTrack.h"

You're including a bit too much.

[emircan, 2015-12-04 04:46:15]

https://codereview.chromium.org/1467103003/diff/500001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h
(right):

https://codereview.chromium.org/1467103003/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/CanvasCaptureMediaStream.h:19:
USING_GARBAGE_COLLECTED_MIXIN(CanvasCaptureMediaStream);
On 2015/12/04 03:55:07, haraken wrote:
> 
> Remove this.

Done.

https://codereview.chromium.org/1467103003/diff/500001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp
(right):

https://codereview.chromium.org/1467103003/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediacapturefromelement/HTMLCanvasElementCapture.cpp:18:
#include "public/platform/WebMediaStreamTrack.h"
On 2015/12/04 03:55:07, haraken wrote:
> 
> You're including a bit too much.

Oops. I am cleaning it up. Thanks for the catch.

[Justin Novosad, 2015-12-04 14:45:29]

LayoutTests lgtm with minor corrections.

https://codereview.chromium.org/1467103003/diff/520001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html
(right):

https://codereview.chromium.org/1467103003/diff/520001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html:11:
description("Exercises the potential events on CanvasCaptureMediaStream.");
Thanks for cleaning up this test. It is much easier to follow now.

https://codereview.chromium.org/1467103003/diff/520001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html:60:
finishJSTest();
Here you are potentially calling finishJSTest before setting jsTestIsAsync.

https://codereview.chromium.org/1467103003/diff/520001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html:62:
gotStream();
Should not call this if there was an exception. Also, functions should be named
based on what they do rather than why/when they're called.

https://codereview.chromium.org/1467103003/diff/520001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html:65:
window.successfullyParsed = true;
This is not necessary.

[Justin Novosad, 2015-12-04 14:49:24]

https://codereview.chromium.org/1467103003/diff/520001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html
(right):

https://codereview.chromium.org/1467103003/diff/520001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html:60:
finishJSTest();
On 2015/12/04 14:45:29, Justin Novosad wrote:
> Here you are potentially calling finishJSTest before setting jsTestIsAsync.

FWIW, this ordering does not actually matter other than for readability.

[no sievers, 2015-12-04 18:11:00]

content/renderer/renderer_blink_platform_impl.* and gyp lgtm

[emircan, 2015-12-04 18:49:37]

Thanks.

https://codereview.chromium.org/1467103003/diff/520001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html
(right):

https://codereview.chromium.org/1467103003/diff/520001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html:60:
finishJSTest();
On 2015/12/04 14:49:24, Justin Novosad wrote:
> On 2015/12/04 14:45:29, Justin Novosad wrote:
> > Here you are potentially calling finishJSTest before setting jsTestIsAsync.
> 
> FWIW, this ordering does not actually matter other than for readability.

Moving it to the beginning of the file.

https://codereview.chromium.org/1467103003/diff/520001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html:62:
gotStream();
On 2015/12/04 14:45:29, Justin Novosad wrote:
> Should not call this if there was an exception. Also, functions should be
named
> based on what they do rather than why/when they're called.

Renaming it to playMediaStream() and moving inside try{} block.

https://codereview.chromium.org/1467103003/diff/520001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-events-and-exceptions.html:65:
window.successfullyParsed = true;
On 2015/12/04 14:45:29, Justin Novosad wrote:
> This is not necessary.

Done.

[emircan, 2015-12-04 19:36:45]

Patchset #16 (id:560001) has been deleted

[emircan, 2015-12-04 19:38:46]

Patchset #16 (id:580001) has been deleted

[Rick Byers, 2015-12-04 19:48:32]

rbyers@chromium.org changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2015-12-04 19:48:33]

Source/modules/mediacapturefromelement/DEPS LGTM
(presubmit seems over-concerned about this - crbug.com/545323?)

[cpu_(ooo_6.6-7.5), 2015-12-04 20:04:25]

cpu@chromium.org changed reviewers:
+ cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2015-12-04 20:04:25]

rubberstamp lgtm given
https://code.google.com/p/chromium/issues/detail?id=545323

[emircan, 2015-12-04 22:57:32]

Patchset #16 (id:600001) has been deleted

[emircan, 2015-12-05 01:18:15]

Patchset #17 (id:630033) has been deleted

[emircan, 2015-12-05 03:06:06]

Patchset #16 (id:620001) has been deleted

[emircan, 2015-12-07 10:58:30]

The CQ bit was checked by emircan@chromium.org

[emircan, 2015-12-07 10:58:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, perkj@chromium.org, haraken@chromium.org,
junov@chromium.org, sievers@chromium.org, cpu@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1467103003/#ps670001
(title: "Added new interface to LayoutTests.")

[commit-bot: I haz the power, 2015-12-07 10:59:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1467103003/670001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1467103003/670001

[commit-bot: I haz the power, 2015-12-07 11:27:02]

Description was changed from

==========
Basic use implementation for MediaStream from Canvas: captureStream()

This CL handles captureStream() case where frame rate is omitted and
"the user agent should capture new frames each time that the content
of the canvas changes". HTMLCanvasElement "pushes" new frames to
AutoCanvasDrawListener when it is ready. 
 
Please look at the detailed updated design doc here: https://goo.gl/BFJY52 
BUG=254990
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

to

==========
Basic use implementation for MediaStream from Canvas: captureStream()

This CL handles captureStream() case where frame rate is omitted and
"the user agent should capture new frames each time that the content
of the canvas changes". HTMLCanvasElement "pushes" new frames to
AutoCanvasDrawListener when it is ready. 
 
Please look at the detailed updated design doc here: https://goo.gl/BFJY52 
BUG=254990
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

[commit-bot: I haz the power, 2015-12-07 11:27:03]

Committed patchset #17 (id:670001)

[commit-bot: I haz the power, 2015-12-07 11:27:36]

Description was changed from

==========
Basic use implementation for MediaStream from Canvas: captureStream()

This CL handles captureStream() case where frame rate is omitted and
"the user agent should capture new frames each time that the content
of the canvas changes". HTMLCanvasElement "pushes" new frames to
AutoCanvasDrawListener when it is ready. 
 
Please look at the detailed updated design doc here: https://goo.gl/BFJY52 
BUG=254990
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...
==========

to

==========
Basic use implementation for MediaStream from Canvas: captureStream()

This CL handles captureStream() case where frame rate is omitted and
"the user agent should capture new frames each time that the content
of the canvas changes". HTMLCanvasElement "pushes" new frames to
AutoCanvasDrawListener when it is ready.

Please look at the detailed updated design doc here: https://goo.gl/BFJY52
BUG=254990
TEST=Ran the demos listed in [0] with MediaCaptureFromElement flag enabled.
[0]
https://docs.google.com/document/d/1JmWfOtUP6ZqsYJ--U8y0OtHkBt-VyjX4N-JqIjb1t...

Committed: https://crrev.com/15d1bb360b49621c833febd823b68954de99802b
Cr-Commit-Position: refs/heads/master@{#363451}
==========

[commit-bot: I haz the power, 2015-12-07 11:27:38]

Patchset 17 (id:??) landed as
https://crrev.com/15d1bb360b49621c833febd823b68954de99802b
Cr-Commit-Position: refs/heads/master@{#363451}

[Peter Mayo, 2015-12-07 16:20:52]

On 2015/12/07 11:27:38, commit-bot: I haz the power wrote:
> Patchset 17 (id:??) landed as
> https://crrev.com/15d1bb360b49621c833febd823b68954de99802b
> Cr-Commit-Position: refs/heads/master@{#363451}

I get 
In file included from ../../content/renderer/renderer_blink_platform_impl.cc:61:
../../content/renderer/media/canvas_capture_handler.h:44:34: error:
[chromium-style] Overriding method must be marked with 'override' or 'final'.
  virtual ~CanvasCaptureHandler();


Anyone else see something like that?

[pdr., 2015-12-07 18:11:41]

On 2015/12/07 at 16:20:52, petermayo wrote:
> On 2015/12/07 11:27:38, commit-bot: I haz the power wrote:
> > Patchset 17 (id:??) landed as
> > https://crrev.com/15d1bb360b49621c833febd823b68954de99802b
> > Cr-Commit-Position: refs/heads/master@{#363451}
> 
> I get 
> In file included from
../../content/renderer/renderer_blink_platform_impl.cc:61:
> ../../content/renderer/media/canvas_capture_handler.h:44:34: error:
[chromium-style] Overriding method must be marked with 'override' or 'final'.
>   virtual ~CanvasCaptureHandler();
> 
> 
> Anyone else see something like that?

This is now happening on the asan bots, see:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/b...

[emircan, 2015-12-07 18:44:47]

On 2015/12/07 18:11:41, pdr wrote:
> On 2015/12/07 at 16:20:52, petermayo wrote:
> > On 2015/12/07 11:27:38, commit-bot: I haz the power wrote:
> > > Patchset 17 (id:??) landed as
> > > https://crrev.com/15d1bb360b49621c833febd823b68954de99802b
> > > Cr-Commit-Position: refs/heads/master@{#363451}
> > 
> > I get 
> > In file included from
> ../../content/renderer/renderer_blink_platform_impl.cc:61:
> > ../../content/renderer/media/canvas_capture_handler.h:44:34: error:
> [chromium-style] Overriding method must be marked with 'override' or 'final'.
> >   virtual ~CanvasCaptureHandler();
> > 
> > 
> > Anyone else see something like that?
> 
> This is now happening on the asan bots, see:
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/b...

I applied  a fix on: https://codereview.chromium.org/1510593002/