Closed Bug 1032848 Opened 10 years ago Closed 9 years ago

Add mediastream = canvas.CaptureStream() or equivalent

Categories

(Core :: WebRTC, defect, P1)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: jesup, Assigned: pehrsons)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 19 obsolete files)

MozReview Request: Bug 1032848 - Part 1. Implement WebIDL for HTMLCanvasElement::CaptureStream. r=smaug,mt
39 bytes, text/x-review-board-request
jgilbert
: review+
Details
MozReview Request: Bug 1032848 - Part 2. Implement HTMLCanvasElement::CaptureStream. r=mt,jesup,jgilbert,gwright
39 bytes, text/x-review-board-request
jgilbert
: review+
Details
MozReview Request: Bug 1032848 - Part 3. Add tests for HTMLCanvasElement::CaptureStream. r=mt,jgilbert,jesup
39 bytes, text/x-review-board-request
jgilbert
: review+
Details 
We *really* should have the ability to capture a canvas into a MediaStream.

We'd want to be able to capture a frame either on an explicit strobe by JS (canvas.CaptureFrame()?) or off a timer - I don't trust setTimeout() to be non-janky, so we may want to consider telling it to capture off a timer, especially if we're using WebGL-ish stuff to write into it.
I think that the way to do this is to add a captureStream method to the element.  Though the implementation would be different to that of HTMLMediaElement, the user interface is the same.  The captureStreamUntilEnded variant isn't applicable here though.
Yes, sorry, I put it in the title - stream = canvas.captureStream().  But then we need to decide when to capture frames from the canvas, which was what I was speaking to.  (I.e. draw a frame, capture it, repeat (or when the user tells you to) is one model; the other is continual update (but don't do it if nothing changed!)
Have you proposed this to whatwg?
The proposal has been floated in the webapps/webrtc media capture task force in the W3C.  There's conditional agreement to accept some work, once that work is updated.  They have an old proposal that was coupled to the mozilla proposal for media processing, but they want a new one.  An updated spec proposal is on my TODO list, but it's still low on the priority list.
This is interesting to a number of folks doing WebGL/asm.js-based games, who want to do live streaming of the game content.  Any further updates on where this lies priority wise?
Randell asked for some webidl on this.
Attachment #8531802 - Flags: feedback?(martin.thomson)
Comment on attachment 8531802 [details] [diff] [review]
webidl for canvas.captureStream()

Review of attachment 8531802 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLCanvasElement.cpp
@@ +398,5 @@
> +  if (!stream) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }
> +  return stream.forget();

A single stream isn't going to work in the form that I'm going to propose.

::: dom/webidl/HTMLCanvasElement.webidl
@@ +51,5 @@
> +
> +  // The frequency at which to generate frames in any stream(s) captured from
> +  // this canvas, in milliseconds. When zero (the default), no frames are
> +  // generated. May be set before and after calls to mozCaptureStream().
> +  attribute unsigned long mozCaptureInterval;

I think that you want to present this as a frame rate constraint (or setting) on the track.  The initial value can be passed to capture stream.

@@ +56,5 @@
> +
> +  [Throws]
> +  // Generate a new frame in any stream(s) captured from this canvas. May be
> +  // be used to drive frames manually.
> +  void mozStrobe();

Using the image capture API might make more sense.  That is you generate a stream with a frame rate of 0, then generate frame grabs as you need them.  Manually forcing frame progression seems like an odd feature to include as a first step anyway, maybe it can wait.
Attachment #8531802 - Flags: feedback?(martin.thomson)
(In reply to Martin Thomson [:mt] from comment #7)
> Comment on attachment 8531802 [details] [diff] [review]
> webidl for canvas.captureStream()
> 
> Review of attachment 8531802 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/html/HTMLCanvasElement.cpp
> @@ +398,5 @@
> > +  if (!stream) {
> > +    aRv.Throw(NS_ERROR_FAILURE);
> > +    return nullptr;
> > +  }
> > +  return stream.forget();
> 
> A single stream isn't going to work in the form that I'm going to propose.

In theory this perhaps should be a track - but right now we can't construct MediaStreams from tracks. 

> ::: dom/webidl/HTMLCanvasElement.webidl
> @@ +51,5 @@
> > +
> > +  // The frequency at which to generate frames in any stream(s) captured from
> > +  // this canvas, in milliseconds. When zero (the default), no frames are
> > +  // generated. May be set before and after calls to mozCaptureStream().
> > +  attribute unsigned long mozCaptureInterval;
> 
> I think that you want to present this as a frame rate constraint (or
> setting) on the track.  The initial value can be passed to capture stream.

So long as it can be varied as needed (and set to infinite/paused).  If it's a frame rate, please make sure it's floating point.

> @@ +56,5 @@
> > +
> > +  [Throws]
> > +  // Generate a new frame in any stream(s) captured from this canvas. May be
> > +  // be used to drive frames manually.
> > +  void mozStrobe();
> 
> Using the image capture API might make more sense.  That is you generate a
> stream with a frame rate of 0, then generate frame grabs as you need them. 
> Manually forcing frame progression seems like an odd feature to include as a
> first step anyway, maybe it can wait.

image capture API though doesn't insert frames into the stream, IIRC, so it's a someone confusing reuse I think.  Manual frame progression makes total sense for synthetic sources.  Think animation/games (you want to finish drawing the frame, then capture a frame and repeat).  Or take a frame of video, analyze it, add reindeer antlers, and tell stream to capture a new frame.  Or a slideshow of images (load image into canvas, strobe, then repeat with next image).  However, putting it on the track may make sense - it depends on you you view the canvas: if there are multiple streams sourced from the same canvas, do you see it as a single, unified source feeding tracks, or as something sampled.
(In reply to Randell Jesup [:jesup] from comment #8)
> In theory this perhaps should be a track - but right now we can't construct
> MediaStreams from tracks. 

Yes, the stream does only get in the way, but we've a well-established pattern of use that relies on streams, not tracks.  I don't see much value in changing that.  Surfacing a stream enables easier use of those other APIs.

> So long as it can be varied as needed (and set to infinite/paused).  If it's
> a frame rate, please make sure it's floating point.

    ConstrainDouble    frameRate;

> image capture API though doesn't insert frames into the stream, IIRC, so
> it's a someone confusing reuse I think.  Manual frame progression makes
> total sense for synthetic sources.  Think animation/games (you want to
> finish drawing the frame, then capture a frame and repeat).  Or take a frame
> of video, analyze it, add reindeer antlers, and tell stream to capture a new
> frame.  Or a slideshow of images (load image into canvas, strobe, then
> repeat with next image).  However, putting it on the track may make sense -
> it depends on you you view the canvas: if there are multiple streams sourced
> from the same canvas, do you see it as a single, unified source feeding
> tracks, or as something sampled.

Most of what you are describing is enabled by the canvas.getContext('2d').getImageData()  If this is your use case, then I think that we are best served without this API.

Note that it is entirely reasonable for the browser to wait until a draw pass has completed before grabbing frames (so if the JS is busy, you get a non-regular frame interval).
Thanks for taking this one, Andreas!

Jib -- FYI: Andreas may need some assistance from you with the DOM work for this.  (He's finishing another bug first.)
Assignee: nobody → pehrsons
This is a fairly basic implementation for the frame rate case, per https://w3c.github.io/mediacapture-fromelement/index.html.

An nsITimer is hooked up to the CanvasCaptureMediaStream to trigger frames in case the user requested a frame rate > 0.

To improve performance when streaming webgl canvases we should implement a readback using PBOs, see e.g., bug 719168.
For a frame rate case it should be fairly straightforward.
Side effect: images will be delayed until the next frame.

The delay would get significant if the time between frames is long. Another timer could make sure that the image is read back in reasonable time.

Martin: A couple of questions arose as well, that I couldn't find explained in the spec proposal:
* What's expected if a user calls captureStream multiple times, especially with different arguments?
* Should requestFrame() have any effect in other cases than a 0-frameRate?
Flags: needinfo?(martin.thomson)
Two tests (2d and webgl) for streaming a canvas stream over a PeerConnection. I intend to add some more basic tests as well that don't involve a PeerConnection.
Comment on attachment 8577922 [details] [diff] [review]
Part 1. Implement WebIDL for HTMLCanvasElement::MozCaptureStream

Review of attachment 8577922 [details] [diff] [review]:
-----------------------------------------------------------------

So glad someone's working on this!

::: dom/webidl/HTMLCanvasElement.webidl
@@ +44,5 @@
>    void mozFetchAsStream(nsIInputStreamCallback callback, optional DOMString? type = null);
>             attribute PrintCallback? mozPrintCallback;
> +
> +  [Throws, UnsafeInPrerendering]
> +  CanvasCaptureMediaStream mozCaptureStream(optional double frameRate);

Let's not use a prefix again. That's a mistake, we should just hide this behind a pref until it's ready to ship.

There shouldn't be any problem calling this multiple times with different frame rates. We'll just be capturing different frames into different streams.
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #13)
> To improve performance when streaming webgl canvases we should implement a
> readback using PBOs, see e.g., bug 719168.
> For a frame rate case it should be fairly straightforward.
> Side effect: images will be delayed until the next frame.
> 
> The delay would get significant if the time between frames is long. Another
> timer could make sure that the image is read back in reasonable time.

We don't have to readback to get data into the MediaStream. MediaStream video tracks carry Images and we can create an Image type that wraps a copy of a GPU canvas frame --- if it doesn't already exist.

AFAIK we won't have to actually read back until something consumes the Image. For MediaRecorder, readback latency isn't a problem. For a PeerConnection streaming a series of a GPU-backed Images, that's just a hard problem. I guess we can initiate an async readback as soon as the Image is acquired from the canvas and then the PeerConnection implementation will just have to block.
Yeah, roc covered the readback delay issue.  If we are on a slow machine, then we will see some latency, that's no problem.  In the real-time cases, we should check that RTP is timestamped at the time the readback finishes though.

The effect of calling captureStream() multiple times will no doubt be that you have multiple people reading from the same canvas.  If that is a major issue, you are permitted to limit the number of streams and throw an exception; let me know if this happens and I can write up the scenario in the spec.

However, I'd expect that you would just have two overlapping series of timers, both pulling independently.  You could optimize that by doing one or both of: creating a shared image if the two timers happen to pull at approximately the same time, or even trying to align timers so that you can share images.  I wouldn't bother with either hack unless it is either trivial to do so (e.g., return the last Image if one was provided since the last draw), or you see widespread use of independent captureStream calls AND if there are serious performance problems where that happens.

My hope was that requestFrame() would just request an additional frame when it is called, even when there is a timer running.  If that is easy enough to do, I can be more specific.
Flags: needinfo?(martin.thomson)
Rank: 19
Flags: firefox-backlog+
Priority: -- → P1
(In reply to Martin Thomson [:mt] from comment #17)
> My hope was that requestFrame() would just request an additional frame when
> it is called, even when there is a timer running.  If that is easy enough to
> do, I can be more specific.

This will be fine. I'll see where the other issues are going once we have solved the webgl snapshots.


(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> We don't have to readback to get data into the MediaStream. MediaStream
> video tracks carry Images and we can create an Image type that wraps a copy
> of a GPU canvas frame --- if it doesn't already exist.
> 
> AFAIK we won't have to actually read back until something consumes the
> Image. For MediaRecorder, readback latency isn't a problem. For a
> PeerConnection streaming a series of a GPU-backed Images, that's just a hard
> problem. I guess we can initiate an async readback as soon as the Image is
> acquired from the canvas and then the PeerConnection implementation will
> just have to block.

I played around with this a bit and ran into numerous issues with the Compositor - haven't even looked at reading the images back yet. It works for the non-e10s case (though I suspect something is a bit off because performance seems quite bad). E10s I haven't gotten working at all yet, though it seems to me that we're always doing readback there on desktop, and passing textures to the parent process through shmem.

The main problem here (compositing these images) is that we're going through ImageClient rather than CanvasClient (where all the existing Canvas-aware logic lies). If we want to pass around a reference to the data on the GPU rather than reading it back, we'll need to be sure we're not exhausting resources. There are many unknowns here:
There could be multiple consumers of this Image; e.g., video element needs to composit, mediarecorder needs to read back, PeerConnection needs to read back.
Any of these consumers can occur in any number, the same image could even occur in multiple streams.
There will be some buffering as the image passes through MediaStreamGraph, plus an unknown period during which the consumers keep a reference around - I assume we don't want to hold up GPU memory for very long but I am not sure if any guarantees can be made on the lifetime of the image.

I think the easiest way forward, with regard to implementation and my own sanity, would be to have the canvas context serve up an image (could be cached) that immediately starts an async readpixels. The image data will then be available to ImageClient and any other consumers that need it in software when requested - hopefully the reading is done when the image has passed through MediaStreamGraph or we'll block.

Nicolas, Jeff, please chime in with your expertise here on what is feasible. Can we pass around an Image wrapping a SharedSurface and use it where needed (ImageClient is the most tricky case afaik), or will this path lead to insanity? Any other way to do this than readback?

And out of curiosity, is there a plan for compositing webgl in e10s without readbacks? Is it even possible?
Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(jgilbert)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #18)
> The main problem here (compositing these images) is that we're going through
> ImageClient rather than CanvasClient (where all the existing Canvas-aware
> logic lies). If we want to pass around a reference to the data on the GPU
> rather than reading it back, we'll need to be sure we're not exhausting
> resources. There are many unknowns here:
> There could be multiple consumers of this Image; e.g., video element needs
> to composit, mediarecorder needs to read back, PeerConnection needs to read
> back.
> Any of these consumers can occur in any number, the same image could even
> occur in multiple streams.

This is already the case with ImageClient (multiple consumers). It was quite the headache to get to work properly, but we haven't had issues with this for a little while.

> There will be some buffering as the image passes through MediaStreamGraph,
> plus an unknown period during which the consumers keep a reference around -
> I assume we don't want to hold up GPU memory for very long but I am not sure
> if any guarantees can be made on the lifetime of the image.

The video pipeline already buffers gpu-side video frames with some decoders. I don't know if this is going to be an issue.

> 
> I think the easiest way forward, with regard to implementation and my own
> sanity, would be to have the canvas context serve up an image (could be
> cached) that immediately starts an async readpixels. The image data will
> then be available to ImageClient and any other consumers that need it in
> software when requested - hopefully the reading is done when the image has
> passed through MediaStreamGraph or we'll block.

Things should be made so that we eventually get in a situation where we only read-back when something consumes textures on the CPU. If the path to get there is to have a first implementation that works but always reads back, I think it's fine (just like WebGL+e10s on some platforms that don't ship cross-process yet).

> And out of curiosity, is there a plan for compositing webgl in e10s without
> readbacks? Is it even possible?

Yeah definitely. Reading back every frame won't give us competitive performance, so we definitely want to avoid it whenever we can. On Windows+D3D11 we share GPU textures across processes (and threads) through DXGI, on B2G, we use gralloc textures, on Linux there are X pixmaps with the texture from pixmap extension but since we don't ship with acceleratied compositing on Linux, we end up reading back there. On mac I don't know for sure (there's something called IOSurface but I don't know how much we use it or want to use it).

Long term, the idea is to merge SharedSurface and TextureClient/Host. SharedSurface and TextureClient are very overlapping concepts that were implemented around the same time with deadline constraints that enforced that one project shouldn't wait on the other. TextureClient/Host are built to support the tricky multiple-consumer cases, so if TextureClient is used to transport the textures on the child side and if the texture is fully owned by the TextureClient, the hardest bits of sharing are already implemented.

TextureClient is currently the only way we have to share textures cross-process, so I suggest you build things on top of it. TextureClient+ImageClient supports multiple consumers pretty well.
Perhaps, if you manage to get a TextureClient out of the canvas and put it in an ImageContainer (all of the consumers seem to know how to talk to ImageContainers), it will mostly work (famous last words) on the condition that the TextureClient fully owns the underlying gpu or cpu texture data (so you may want to copy whatever's coming out of the Canvas into a TextureClient if the canvas code insists on managing its own memory).

In any case my advice would be to not rely too much on SharedSurface because it doesn't support managing the lifetime of a texture that has multiple consumers on different threads, and that is pretty hard to get to work well in gecko.
Flags: needinfo?(nical.bugzilla)
Mostly rebased.
Attachment #8577922 - Attachment is obsolete: true
Thanks for a very thorough answer earlier Nicolas.

This patch contains a new Image class that wraps a SharedSurface but always reads the pixels out (for now, I want to at least do an async read in the future) and uses the resulting SourceSurface wherever we need pixel data access (like WebRTC) or if the SharedSurface cannot be composited through a SharedSurfaceTextureClient (as in e10s or non-OMTC).

Skipping the readback altogether seems tricky enough for me to defer it to sometime in the future. We have the compositor cases, most notably e10s that needs to do cross-process stuff complicating this. We have pure software cases like MediaPipeline running on a background thread and requiring pixel data access to also complicate things.

Any opinions on this approach from you?
Attachment #8577935 - Attachment is obsolete: true
Attachment #8582353 - Flags: review?(nical.bugzilla)
Tests are updated and a bit cleaner.

Still only two tests, sending/receiving the CanvasCaptureStream over a PeerConnection for a 2D (1) and a WebGL (2) canvas.
Attachment #8577936 - Attachment is obsolete: true
Wups - missed including two new files. Previous comment on part 2 still stands.
Attachment #8582353 - Attachment is obsolete: true
Attachment #8582353 - Flags: review?(nical.bugzilla)
Attachment #8582355 - Flags: feedback?(nical.bugzilla)
I might be seeing a bug on linux with e10s. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1100d60c699

Looks like a race of sorts between ImageClient on the ImageBridgeChild thread and the CanvasClient on the main thread.

ImageClient Stack trace on my virtual machine, debugging the child process, on ImageBridgeChild thread:
> #0  0x00007ffff13cfcc9 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> #1  0x00007ffff13d30d8 in __GI_abort () at abort.c:89
> #2  0x00007ffff13c8b86 in __assert_fail_base (fmt=0x7ffff1519830 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7fffecb00390 "!xcb_xlib_threads_sequence_lost", 
    file=file@entry=0x7fffecb001db "../../src/xcb_io.c", line=line@entry=635, function=function@entry=0x7fffecb00657 "_XReply") at assert.c:92
> #3  0x00007ffff13c8c32 in __GI___assert_fail (assertion=0x7fffecb00390 "!xcb_xlib_threads_sequence_lost", file=0x7fffecb001db "../../src/xcb_io.c", line=635, function=0x7fffecb00657 "_XReply")
    at assert.c:101
> #4  0x00007fffeca9184c in _XReply () from /usr/lib/x86_64-linux-gnu/libX11.so.6
> #5  0x00007fffeca8d0cd in XSync () from /usr/lib/x86_64-linux-gnu/libX11.so.6
> #6  0x00007ffff323c987 in mozilla::layers::TextureClientX11::AllocateForSurface (this=0x7fffd1cb9b80, aSize=..., aTextureFlags=<optimised out>)
    at /home/pehrsons/mozilla-central/gfx/layers/basic/TextureClientX11.cpp:130
> #7  0x00007ffff327d67d in mozilla::layers::TextureClient::CreateForDrawing (aAllocator=0x7fffe40d02e8, aFormat=<optimised out>, aSize=..., aMoz2DBackend=mozilla::gfx::CAIRO, 
    aMoz2DBackend@entry=mozilla::gfx::NONE, aTextureFlags=mozilla::layers::NO_FLAGS, aAllocFlags=mozilla::layers::ALLOC_DEFAULT) at /home/pehrsons/mozilla-central/gfx/layers/client/TextureClient.cpp:387
> #8  0x00007ffff327d76b in mozilla::layers::CompositableClient::CreateTextureClientForDrawing (this=this@entry=0x7fffdbccfc90, aFormat=<optimised out>, aSize=..., aSize@entry=..., 
    aMoz2DBackend=aMoz2DBackend@entry=mozilla::gfx::NONE, aTextureFlags=aTextureFlags@entry=mozilla::layers::NO_FLAGS, aAllocFlags=mozilla::layers::ALLOC_DEFAULT)
    at /home/pehrsons/mozilla-central/gfx/layers/client/CompositableClient.cpp:214
> #9  0x00007ffff322fa8c in mozilla::layers::CairoImage::GetTextureClient (this=0x7fffdbcf6520, aClient=0x7fffdbccfc90) at /home/pehrsons/mozilla-central/gfx/layers/ImageContainer.cpp:532
> #10 0x00007ffff327e3ba in mozilla::layers::ImageClientSingle::UpdateImage (this=0x7fffdbccfc90, aContainer=0x7fffcfaa2180, aContentFlags=<optimised out>)
    at /home/pehrsons/mozilla-central/gfx/layers/client/ImageClient.cpp:199
> #11 0x00007ffff32a0695 in mozilla::layers::UpdateImageClientNow (aClient=0x7fffdbccfc90, aContainer=0x7fffcfaa2180) at /home/pehrsons/mozilla-central/gfx/layers/ipc/ImageBridgeChild.cpp:421
> #12 0x00007ffff2cd846e in MessageLoop::RunTask (this=0x7fffe22fca40, task=0x7fffd9f96940) at /home/pehrsons/mozilla-central/ipc/chromium/src/base/message_loop.cc:361


CanvasClient stack trace, same machine, different run, main thread:
> #0  0x00007ffff148612d in poll () at ../sysdeps/unix/syscall-template.S:81
> #1  0x00007fffe9d96b72 in ?? () from /usr/lib/x86_64-linux-gnu/libxcb.so.1
> #2  0x00007fffe9d983ff in ?? () from /usr/lib/x86_64-linux-gnu/libxcb.so.1
> #3  0x00007fffe9d98512 in xcb_wait_for_reply () from /usr/lib/x86_64-linux-gnu/libxcb.so.1
> #4  0x00007fffeca9148f in _XReply () from /usr/lib/x86_64-linux-gnu/libX11.so.6
> #5  0x00007fffeca778dd in XGetImage () from /usr/lib/x86_64-linux-gnu/libX11.so.6
> #6  0x00007ffff4ad5f8e in _get_image_surface (surface=0x7fffd1ce6000, interest_rect=interest_rect@entry=0x0, image_out=image_out@entry=0x7fffffffa578, image_rect=image_rect@entry=0x0)
    at /home/pehrsons/mozilla-central/gfx/cairo/cairo/src/cairo-xlib-surface.c:855
> #7  0x00007ffff4ad681d in _cairo_xlib_surface_acquire_source_image (abstract_surface=<optimised out>, image_out=0x7fffffffa690, image_extra=0x7fffffffa6c0)
    at /home/pehrsons/mozilla-central/gfx/cairo/cairo/src/cairo-xlib-surface.c:1390
> #8  0x00007ffff4b06285 in _cairo_surface_acquire_source_image (surface=0x7fffd1ce6000, image_out=image_out@entry=0x7fffffffa690, image_extra=image_extra@entry=0x7fffffffa6c0)
    at /home/pehrsons/mozilla-central/gfx/cairo/cairo/src/cairo-surface.c:1452
> #9  0x00007ffff4aeb6bd in _pixman_image_for_surface (iy=0x7fffffffa784, ix=0x7fffffffa780, extents=<optimised out>, is_mask=0, pattern=0x7fffffffad88)
    at /home/pehrsons/mozilla-central/gfx/cairo/cairo/src/cairo-image-surface.c:1502
> #10 _pixman_image_for_pattern (pattern=pattern@entry=0x7fffffffad88, is_mask=is_mask@entry=0, extents=extents@entry=0x7fffffffa850, tx=tx@entry=0x7fffffffa780, ty=ty@entry=0x7fffffffa784)
    at /home/pehrsons/mozilla-central/gfx/cairo/cairo/src/cairo-image-surface.c:1680
> #11 0x00007ffff4aecee3 in _composite_boxes (extents=0x7fffffffa830, clip=0x0, antialias=CAIRO_ANTIALIAS_DEFAULT, boxes=0x7fffffffaa80, pattern=0x7fffffffad88, op=CAIRO_OPERATOR_SOURCE, 
    dst=0x7fffdedd2960) at /home/pehrsons/mozilla-central/gfx/cairo/cairo/src/cairo-image-surface.c:3016
> #12 _clip_and_composite_boxes (dst=0x7fffdedd2960, op=CAIRO_OPERATOR_SOURCE, src=0x7fffffffad88, boxes=0x7fffffffaa80, antialias=CAIRO_ANTIALIAS_DEFAULT, extents=0x7fffffffa830, clip=0x0)
    at /home/pehrsons/mozilla-central/gfx/cairo/cairo/src/cairo-image-surface.c:3077
> #13 0x00007ffff4aee81b in _cairo_image_surface_paint (abstract_surface=0x7fffdedd2960, op=CAIRO_OPERATOR_SOURCE, source=0x7fffffffad88, clip=0x0)
    at /home/pehrsons/mozilla-central/gfx/cairo/cairo/src/cairo-image-surface.c:3325
> #14 0x00007ffff4b0b84c in _cairo_surface_paint (surface=0x7fffdedd2960, op=CAIRO_OPERATOR_SOURCE, source=0x7fffffffad88, clip=0x0)
    at /home/pehrsons/mozilla-central/gfx/cairo/cairo/src/cairo-surface.c:2109
> #15 0x00007ffff4aee1e2 in _cairo_gstate_fill (gstate=0x7fffd4bbd9c8, path=path@entry=0x7fffd4bbdb68) at /home/pehrsons/mozilla-central/gfx/cairo/cairo/src/cairo-gstate.c:1285
> #16 0x00007ffff4b0f973 in INT__moz_cairo_fill_preserve (cr=cr@entry=0x7fffd4bbd800) at /home/pehrsons/mozilla-central/gfx/cairo/cairo/src/cairo.c:2464
> #17 0x00007ffff4b0f996 in _moz_cairo_fill (cr=0x7fffd4bbd800) at /home/pehrsons/mozilla-central/gfx/cairo/cairo/src/cairo.c:2440
> #18 0x00007ffff31cc701 in mozilla::gfx::DrawTargetCairo::CopySurfaceInternal (this=0x7fffd0f1e680, aSurface=0x7fffd1ce6000, aSource=..., aDest=...)
    at /home/pehrsons/mozilla-central/gfx/2d/DrawTargetCairo.cpp:993
> #19 0x00007ffff31d3996 in mozilla::gfx::DrawTargetCairo::CopySurface (this=0x7fffd0f1e680, aSurface=<optimised out>, aSource=..., aDest=...)
    at /home/pehrsons/mozilla-central/gfx/2d/DrawTargetCairo.cpp:1015
> #20 0x00007ffff32430b6 in mozilla::layers::CopyableCanvasLayer::UpdateTarget (this=this@entry=0x7fffe1197400, aDestTarget=0x7fffd0f1e680)
    at /home/pehrsons/mozilla-central/gfx/layers/CopyableCanvasLayer.cpp:94
> #21 0x00007ffff326c882 in mozilla::layers::CanvasClient2D::Update (this=0x7fffdc72edc0, aSize=..., aLayer=0x7fffe1197400) at /home/pehrsons/mozilla-central/gfx/layers/client/CanvasClient.cpp:102
> #22 0x00007ffff326b6df in mozilla::layers::ClientCanvasLayer::RenderLayer (this=0x7fffe1197400) at /home/pehrsons/mozilla-central/gfx/layers/client/ClientCanvasLayer.cpp:182
> #23 0x00007ffff327343e in mozilla::layers::ClientContainerLayer::RenderLayer (this=0x7fffd0f48c00) at /home/pehrsons/mozilla-central/gfx/layers/client/ClientContainerLayer.h:68
> #24 0x00007ffff327343e in mozilla::layers::ClientContainerLayer::RenderLayer (this=0x7fffd10a1400) at /home/pehrsons/mozilla-central/gfx/layers/client/ClientContainerLayer.h:68
> #25 0x00007ffff3267da1 in mozilla::layers::ClientLayerManager::EndTransactionInternal (this=this@entry=0x7fffe3e53e00, aCallback=aCallback@entry=
    0x7ffff43250e8 <mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*)>, 
    aCallbackData=aCallbackData@entry=0x7fffffffbb30) at /home/pehrsons/mozilla-central/gfx/layers/client/ClientLayerManager.cpp:272
> #26 0x00007ffff326dd7f in mozilla::layers::ClientLayerManager::EndTransaction (this=0x7fffe3e53e00, 
    aCallback=0x7ffff43250e8 <mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*)>, 
    aCallbackData=0x7fffffffbb30, aFlags=mozilla::layers::LayerManager::END_DEFAULT) at /home/pehrsons/mozilla-central/gfx/layers/client/ClientLayerManager.cpp:315
> #27 0x00007ffff4361337 in nsDisplayList::PaintRoot (this=this@entry=0x7fffffffb960, aBuilder=aBuilder@entry=0x7fffffffbb30, aCtx=aCtx@entry=0x0, aFlags=aFlags@entry=13)
    at /home/pehrsons/mozilla-central/layout/base/nsDisplayList.cpp:1763
> #28 0x00007ffff43a6e6f in nsLayoutUtils::PaintFrame (aRenderingContext=aRenderingContext@entry=0x0, aFrame=aFrame@entry=0x7fffe0ebf4a8, aDirtyRegion=..., aBackstop=aBackstop@entry=0, 
    aFlags=<optimised out>) at /home/pehrsons/mozilla-central/layout/base/nsLayoutUtils.cpp:3284
> #29 0x00007ffff43a7955 in PresShell::Paint (this=0x7fffe0e62800, aViewToPaint=aViewToPaint@entry=0x7fffe0fc6510, aDirtyRegion=..., aFlags=aFlags@entry=1)
    at /home/pehrsons/mozilla-central/layout/base/nsPresShell.cpp:6322
> #30 0x00007ffff413c5b6 in nsViewManager::ProcessPendingUpdatesPaint (this=0x7fffe0e8de00, aWidget=aWidget@entry=0x7fffe8d92e30) at /home/pehrsons/mozilla-central/view/nsViewManager.cpp:445
> #31 0x00007ffff413c761 in nsViewManager::ProcessPendingUpdatesForView (this=<optimised out>, aView=<optimised out>, aFlushDirtyRegion=aFlushDirtyRegion@entry=true)
    at /home/pehrsons/mozilla-central/view/nsViewManager.cpp:385
> #32 0x00007ffff413c816 in nsViewManager::ProcessPendingUpdates (this=this@entry=0x7fffe0e8de00) at /home/pehrsons/mozilla-central/view/nsViewManager.cpp:1076
> #33 0x00007ffff430618f in nsRefreshDriver::Tick (this=0x7fffe0e61800, aNowEpoch=aNowEpoch@entry=1427270525402089, aNowTime=...) at /home/pehrsons/mozilla-central/layout/base/nsRefreshDriver.cpp:1715
> #34 0x00007ffff43087ec in mozilla::RefreshDriverTimer::TickDriver (driver=<optimised out>, jsnow=jsnow@entry=1427270525402089, now=..., now@entry=...)
    at /home/pehrsons/mozilla-central/layout/base/nsRefreshDriver.cpp:198
> #35 0x00007ffff43088a4 in mozilla::RefreshDriverTimer::Tick (this=this@entry=0x7fffe3e472c0, jsnow=jsnow@entry=1427270525402089, now=...)

nical, do you see anything obvious I'm doing wrong here or are we likely talking about a bug? It starts occuring when both the Canvas and the Image(s) are being composited. If we're capturing but not playing the stream it's fine.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8582355 [details] [diff] [review]
Part 2. Implement HTMLCanvasElement::CaptureStream

Review of attachment 8582355 [details] [diff] [review]:
-----------------------------------------------------------------

My advice would be to start with something simpler (the trickiness here is in the fact that holding a reference to a SharedSurface in something that is outside of SurfaceStream and used from other threads is a bit like holding a live grenade), see my comment in ImageContainer.h. Once we get this working, the next step would be to go directly from SharedSurface -> TextureClient without, either with an async readback or with no readback at all (the end goal). To do that we'll need some tweaks to SharedSurface to make its lifetime safer.

::: gfx/layers/ImageContainer.h
@@ +871,5 @@
> + * thread.
> + *
> + * TODO: Async readback of pixel data.
> + */
> +class SharedSurfaceImage : public Image {

It would be simpler if you just create a CairoImage out of the canvas by reading the shared surface back when creating the image object. I realize that it will mean a sync readback instead of an async one, but it'll make the lifetime management much simpler as a first step. SharedSurface isn't meant to be kept around outside of the SurfaceStream stuff and we run into issues as soon as we store it anywhere else.

::: gfx/layers/client/ImageClient.cpp
@@ +239,5 @@
>          texture = new EGLImageTextureClient(GetForwarder(),
>                                              mTextureFlags,
>                                              typedImage,
>                                              size);
> +      } else if (image->GetFormat() == ImageFormat::SHARED_SURFACE) {

It would be better if, instead of adding this branch here, you implement GetTextureClient on the new SharedSurfaceImage. This keep the specifics of SharedSurfaceImage in SharedSurfaceImage and make it easier to get into a place where there is no readback (where the Image class is directly backed by a TextureClient). If you cash the TextureClient in the SharedSurfaceImage, you'll also readback once instead of for each Layer that reads from the Image.
This comment doesn't really apply if you choose follow what I propose in my other omment about SharedSurfaceImage.
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #24)
> nical, do you see anything obvious I'm doing wrong here or are we likely
> talking about a bug? It starts occuring when both the Canvas and the
> Image(s) are being composited. If we're capturing but not playing the stream
> it's fine.

I don't see anything obvious that you would be doing wrong. using X from different threads is still a new thing that we don't understand and test very well, so I suggest you make sure we don't create X11TextureClient from the ImageBridgeChild thread to work around this for now.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8582355 [details] [diff] [review]
Part 2. Implement HTMLCanvasElement::CaptureStream

Review of attachment 8582355 [details] [diff] [review]:
-----------------------------------------------------------------

f-, cf. my comment about holding SharedSurface references outside of SurfaceStream. It's the kind of thing that will break intermittently if we do it without fixing SharedSurface's lifetime (it has bitten us recently in TextureCLientSharedSurface and we had to make WebGL transactions with the compositor synchronous because of it). It's not a definitive opinion, but I think there needs to be some work in SharedSurface's lifetime before we can do this.

::: gfx/layers/ImageContainer.h
@@ +871,5 @@
> + * thread.
> + *
> + * TODO: Async readback of pixel data.
> + */
> +class SharedSurfaceImage : public Image {

It would be simpler if you just create a CairoImage out of the canvas by reading the shared surface back when creating the image object. I realize that it will mean a sync readback instead of an async one, but it'll make the lifetime management much simpler as a first step. SharedSurface isn't meant to be kept around outside of the SurfaceStream stuff and we run into issues as soon as we store it anywhere else.

::: gfx/layers/client/ImageClient.cpp
@@ +239,5 @@
>          texture = new EGLImageTextureClient(GetForwarder(),
>                                              mTextureFlags,
>                                              typedImage,
>                                              size);
> +      } else if (image->GetFormat() == ImageFormat::SHARED_SURFACE) {

It would be better if, instead of adding this branch here, you implement GetTextureClient on the new SharedSurfaceImage. This keep the specifics of SharedSurfaceImage in SharedSurfaceImage and make it easier to get into a place where there is no readback (where the Image class is directly backed by a TextureClient). If you cash the TextureClient in the SharedSurfaceImage, you'll also readback once instead of for each Layer that reads from the Image.
This comment doesn't really apply if you choose follow what I propose in my other omment about SharedSurfaceImage.
Attachment #8582355 - Flags: feedback?(nical.bugzilla) → feedback-
(In reply to Nicolas Silva [:nical] from comment #26)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #24)
> > nical, do you see anything obvious I'm doing wrong here or are we likely
> > talking about a bug? It starts occuring when both the Canvas and the
> > Image(s) are being composited. If we're capturing but not playing the stream
> > it's fine.
> 
> I don't see anything obvious that you would be doing wrong. using X from
> different threads is still a new thing that we don't understand and test
> very well, so I suggest you make sure we don't create X11TextureClient from
> the ImageBridgeChild thread to work around this for now.

I'll also point you at the code in media/webrtc/trunk/webrtc/modules/desktop_capture where we dealt with threading issues in X11 with karlt's help
See Also: → 1147777
If this looks good to you Martin, I'll get a DOM peer to review as well.
Attachment #8582350 - Attachment is obsolete: true
Attachment #8583764 - Flags: review?(martin.thomson)
nical (for gfx stuff - basically all the GetInternalImage()), here's as simple as possible. We're doing a readback up front in WebGLContext and also flipping the Y axis.

mt (for spec stuff, mostly DOMCanvasCaptureStream.cpp|.h), this implements all cases of captureStream with a frame rate provided. I'll add the auto-mode in a follow-up bug.

jesup (for stream stuff, also DOMCanvasCaptureStream.cpp|.h), this mimics the behavior in MediaEngineTabVideoSource quite closely. The timer (or requestFrame()) drives the fetching of images from the canvas; NotifyPull always appends the last image to the stream.
Attachment #8582355 - Attachment is obsolete: true
Attachment #8583766 - Flags: review?(nical.bugzilla)
Attachment #8583766 - Flags: review?(martin.thomson)
Attachment #8583766 - Flags: review?(rjesup)
This fixes the X11 issue for me. I could also check for aAllocator->IsImageBridgeChild() but this seems more generic.
Attachment #8583767 - Flags: review?(nical.bugzilla)
Comment on attachment 8583766 [details] [diff] [review]
Part 2. Implement HTMLCanvasElement::CaptureStream

Review of attachment 8583766 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLContext.cpp
@@ +1784,5 @@
> +    }
> +
> +    // Make sure the current frame is available in the front buffer.
> +    gl->MakeCurrent();
> +    PresentScreenBuffer();

PresentScreenBuffer can fail and return false. You should check the return value and handle this case by returning a null image perhaps.
Attachment #8583766 - Flags: review?(nical.bugzilla) → review+
Attachment #8583767 - Flags: review?(nical.bugzilla) → review+
Attachment #8583764 - Flags: review?(martin.thomson) → review+
Comment on attachment 8583766 [details] [diff] [review]
Part 2. Implement HTMLCanvasElement::CaptureStream

Review of attachment 8583766 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not qualified to review the stuff that :nical has reviewed, so I didn't look at that.

When a canvas becomes unreadable, the stream needs to become unusable.  I can't confirm that this is the case without seeing the tests.  Otherwise, this is OK.  Flag me again when you have tests.

::: dom/media/DOMCanvasCaptureMediaStream.cpp
@@ +149,5 @@
> +}
> +
> +// ----------------------------------------------------------------------
> +
> +class TimerDriver : public OutputStreamDriver

I would have used a "Has A" relationship here, rather than an "Is A".  That is, the OutputStreamDriver has a timer (which might be optional if the frame rate is zero).

@@ +185,5 @@
> +    }
> +
> +    RefPtr<Image> image = DOMStream()->Canvas()->GetInternalImage();
> +    if (!image) {
> +      return NS_ERROR_FAILURE;

This is not a recoverable situation, might as well drop the timer when you get an error out of this.
Attachment #8583766 - Flags: review?(martin.thomson)
Comment on attachment 8583766 [details] [diff] [review]
Part 2. Implement HTMLCanvasElement::CaptureStream

Review of attachment 8583766 [details] [diff] [review]:
-----------------------------------------------------------------

I should probably write WebGLContext::GetInternalImage(), I think. (No offense meant, but it's fraught, with a bunch of APIs coming together and needing a lot of WebGL-specific knowledge still)

::: dom/canvas/WebGLContext.cpp
@@ +1784,5 @@
> +    }
> +
> +    // Make sure the current frame is available in the front buffer.
> +    gl->MakeCurrent();
> +    PresentScreenBuffer();

PresentScreenBuffer can also clear the backbuffer, so the compositor will not see the result. I imagine this is not what you want.

@@ +1808,5 @@
> +        return nullptr;
> +    }
> +
> +    GLenum format = surfFormat == SurfaceFormat::B8G8R8A8 ? LOCAL_GL_BGRA
> +                                                          : LOCAL_GL_BGR;

These are generally not valid formats.

@@ +1815,5 @@
> +    DataSourceSurface::MappedSurface map;
> +    if (NS_WARN_IF(!surf->Map(DataSourceSurface::WRITE, &map))) {
> +        return nullptr;
> +    }
> +    front->Surf()->ReadPixels(0, 0, mWidth, mHeight, format, type, map.mData);

This is fallible, and not how the API is meant to be used.

@@ +1818,5 @@
> +    }
> +    front->Surf()->ReadPixels(0, 0, mWidth, mHeight, format, type, map.mData);
> +    surf->Unmap();
> +
> +    // Readback done. Now let's flip the Y axis.

This is /so slow/. Is this spec-mandated? If so, we should try to change the spec.
Attachment #8583766 - Flags: review-
Please get review for WebGL stuff from a WebGL engineer. (For WebGL composition, you want me)
Flags: needinfo?(jgilbert)
Flags: needinfo?(jgilbert)
(In reply to Jeff Gilbert [:jgilbert] from comment #34)
> Comment on attachment 8583766 [details] [diff] [review]
> Part 2. Implement HTMLCanvasElement::CaptureStream
> 
> Review of attachment 8583766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I should probably write WebGLContext::GetInternalImage(), I think. (No
> offense meant, but it's fraught, with a bunch of APIs coming together and
> needing a lot of WebGL-specific knowledge still)

None taken, and please do! I'll admit to some fumbling in the dark here, especially on WebGL-y parts.

> @@ +1818,5 @@
> > +    }
> > +    front->Surf()->ReadPixels(0, 0, mWidth, mHeight, format, type, map.mData);
> > +    surf->Unmap();
> > +
> > +    // Readback done. Now let's flip the Y axis.
> 
> This is /so slow/. Is this spec-mandated? If so, we should try to change the
> spec.

It's not. The plan is to optimize this in a follow-up bug. Probably doing an async readback as a first step.
Except having the compositor as a consumer here (through ImageClient) we also have stuff like MediaRecorder and MediaPipeline (WebRTC) needing direct access to the image data on background threads.
(In reply to Martin Thomson [:mt] from comment #33)
> Comment on attachment 8583766 [details] [diff] [review]
> Part 2. Implement HTMLCanvasElement::CaptureStream
> 
> Review of attachment 8583766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/DOMCanvasCaptureMediaStream.cpp
> @@ +149,5 @@
> > +}
> > +
> > +// ----------------------------------------------------------------------
> > +
> > +class TimerDriver : public OutputStreamDriver
> 
> I would have used a "Has A" relationship here, rather than an "Is A".  That
> is, the OutputStreamDriver has a timer (which might be optional if the frame
> rate is zero).

My idea is to use a TimerDriver when we need a timer to drive the frame rate, and another sub class (to be implemented in a follow-up) when we need the auto mode - i.e., no parameter provided to captureStream(). Not sure how that would be hooked up yet, though.

How does this sound to you?

> @@ +185,5 @@
> > +    }
> > +
> > +    RefPtr<Image> image = DOMStream()->Canvas()->GetInternalImage();
> > +    if (!image) {
> > +      return NS_ERROR_FAILURE;
> 
> This is not a recoverable situation, might as well drop the timer when you
> get an error out of this.

Sure!
Flags: needinfo?(martin.thomson)
Here's an update of the tests. Still WIP.

Now testing that we cannot captureStream() on a 2d canvas that is not origin-clean, and that captured streams from a webgl canvas draw properly in the various modes (framerates 10 and 0, with requestFrame(), for now).

Jeff, is there any way to make a webgl canvas write only? As I understood things it is illegal to upload cross origing textures in the first place, so it will never reach a write-only state.

I still have to write a test for 2d canvases, testing that the modes of capture work and that making it write-only stops sending further frames to the stream.
Attachment #8582354 - Attachment is obsolete: true
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #37)
> (In reply to Martin Thomson [:mt] from comment #33)
> > Comment on attachment 8583766 [details] [diff] [review]
> > Part 2. Implement HTMLCanvasElement::CaptureStream
> > 
> > Review of attachment 8583766 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/media/DOMCanvasCaptureMediaStream.cpp
> > @@ +149,5 @@
> > > +}
> > > +
> > > +// ----------------------------------------------------------------------
> > > +
> > > +class TimerDriver : public OutputStreamDriver
> > 
> > I would have used a "Has A" relationship here, rather than an "Is A".  That
> > is, the OutputStreamDriver has a timer (which might be optional if the frame
> > rate is zero).
> 
> My idea is to use a TimerDriver when we need a timer to drive the frame
> rate, and another sub class (to be implemented in a follow-up) when we need
> the auto mode - i.e., no parameter provided to captureStream(). Not sure how
> that would be hooked up yet, though.
> 
> How does this sound to you?

That's a pretty good answer.  Do you have a plan for how to do the auto mode?

I assume that you plan to hook up a default frame rate until that follow-up is implemented.
Flags: needinfo?(martin.thomson)
Comment on attachment 8584418 [details] [diff] [review]
Part 3. Add tests for HTMLCanvasElement::CaptureStream

Review of attachment 8584418 [details] [diff] [review]:
-----------------------------------------------------------------

I think that you need to check the case where the canvas stops being origin-clean after the stream has been created.  I have some awful code in dom/media/tests/mochitest/blacksilence.js that tests whether a video stream is producing black frames.  You could use that; improvements there would be greatly appreciated, of course.
Comment on attachment 8583766 [details] [diff] [review]
Part 2. Implement HTMLCanvasElement::CaptureStream

Review of attachment 8583766 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with TSAN issue and nits dealt with

::: dom/html/HTMLCanvasElement.cpp
@@ +419,5 @@
> +    return nullptr;
> +  }
> +
> +  nsRefPtr<DOMCanvasCaptureMediaStream> stream =
> +    DOMCanvasCaptureMediaStream::CreateSourceStream(window, this);

I presume this can't fail?

@@ +424,5 @@
> +
> +  nsRefPtr<nsIPrincipal> principal = NodePrincipal();
> +  stream->CombineWithPrincipal(principal);
> +
> +  TrackID videoTrackId = 1;

Perhaps a define/etc?

::: dom/media/DOMCanvasCaptureMediaStream.cpp
@@ +30,5 @@
> +  {
> +    MOZ_ASSERT(mDriver);
> +    MOZ_ASSERT(mSourceStream);
> +  }
> +  void Forget() {

blank lines between methods

@@ +35,5 @@
> +    mDriver = nullptr;
> +  }
> +  virtual void NotifyPull(MediaStreamGraph* aGraph, StreamTime aDesiredTime) override
> +  {
> +    nsRefPtr<OutputStreamDriver> driver = mDriver;

there's a TSAN race between Forget() and NotifyPull() here unless we know pulls have for-sure stopped.  Needs a lock or maybe Atomic<> somewhere

@@ +48,5 @@
> +protected:
> +  ~StreamListener() { }
> +
> +private:
> +  OutputStreamDriver* mDriver;

Comment as to why a raw ptr is safe here, especially thread-safety

@@ +53,5 @@
> +  nsRefPtr<SourceMediaStream> mSourceStream;
> +};
> +
> +OutputStreamDriver::OutputStreamDriver(DOMCanvasCaptureMediaStream* aDOMStream,
> +                   const TrackID& aTrackId)

indent

@@ +69,5 @@
> +OutputStreamDriver::~OutputStreamDriver()
> +{
> +  if (mStreamListener) {
> +    mStreamListener->Forget();
> +  }

Can the mStreamListener get deleted before the next NotifyPull()?  I presume no (that MSG is holding a ref)

@@ +178,5 @@
> +  {
> +    MOZ_ASSERT(DOMStream());
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    // mDOMStream can't be killed while we're on main thread

I presume DOMStream()->Canvas() can't either

::: dom/media/DOMCanvasCaptureMediaStream.h
@@ +60,5 @@
> +
> +private:
> +  DOMCanvasCaptureMediaStream* mDOMStream;
> +  nsRefPtr<layers::Image> mImage;
> +  Mutex mMutex;

Indicate with comments and blocking/order which things are protected by the mutex
Attachment #8583766 - Flags: review?(rjesup) → review+
(In reply to Martin Thomson [:mt] from comment #39)
> That's a pretty good answer.  Do you have a plan for how to do the auto mode?
> 
> I assume that you plan to hook up a default frame rate until that follow-up
> is implemented.

My only thought so far is to let the canvas context register a callback to be called whenever it draws something. The auto driver could have a one-off timer to defer the frame capture to the next stable state (and "cache" multiple callbacks before the stable state is reached).

I had actually planned to leave it throwing NS_ERROR_NOT_IMPLEMENTED but I agree using a frame rate is better. I'll do 30 FPS.
(In reply to Martin Thomson [:mt] from comment #40)
> Comment on attachment 8584418 [details] [diff] [review]
> Part 3. Add tests for HTMLCanvasElement::CaptureStream
> 
> Review of attachment 8584418 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think that you need to check the case where the canvas stops being
> origin-clean after the stream has been created.  I have some awful code in
> dom/media/tests/mochitest/blacksilence.js that tests whether a video stream
> is producing black frames.  You could use that; improvements there would be
> greatly appreciated, of course.

Only testing WebGL so far and as I understood it a WebGLContext cannot stop being origin-clean: https://www.khronos.org/registry/webgl/specs/1.0/#4.2

I'll test it properly with a 2d context.
(In reply to Randell Jesup [:jesup] from comment #41)
> Comment on attachment 8583766 [details] [diff] [review]
> Part 2. Implement HTMLCanvasElement::CaptureStream
> 
> Review of attachment 8583766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with TSAN issue and nits dealt with
> 
> ::: dom/html/HTMLCanvasElement.cpp
> @@ +419,5 @@
> > +    return nullptr;
> > +  }
> > +
> > +  nsRefPtr<DOMCanvasCaptureMediaStream> stream =
> > +    DOMCanvasCaptureMediaStream::CreateSourceStream(window, this);
> 
> I presume this can't fail?

It can't, but I'll add a guard anyway to be future-proof.


> @@ +424,5 @@
> > +
> > +  nsRefPtr<nsIPrincipal> principal = NodePrincipal();
> > +  stream->CombineWithPrincipal(principal);
> > +
> > +  TrackID videoTrackId = 1;
> 
> Perhaps a define/etc?

We only use it in one place (here) so I'd prefer to keep it within this scope if possible.


> ::: dom/media/DOMCanvasCaptureMediaStream.cpp
> @@ +30,5 @@
> > +  {
> > +    MOZ_ASSERT(mDriver);
> > +    MOZ_ASSERT(mSourceStream);
> > +  }
> > +  void Forget() {
> 
> blank lines between methods

Fixed.


> @@ +35,5 @@
> > +    mDriver = nullptr;
> > +  }
> > +  virtual void NotifyPull(MediaStreamGraph* aGraph, StreamTime aDesiredTime) override
> > +  {
> > +    nsRefPtr<OutputStreamDriver> driver = mDriver;
> 
> there's a TSAN race between Forget() and NotifyPull() here unless we know
> pulls have for-sure stopped.  Needs a lock or maybe Atomic<> somewhere

I was looking into NS_INLINE_DECL_THREADSAFE_REFCOUNTING a bit when going through this. Is my understanding correct that we need to guard mDriver because it's a raw pointer, and that we don't need to guard OutputStreamDriver::Image because it has threadsafe refcounting and is a RefPtr?

I've gotten rid of the Mutex for mImage now, but added one for mDriver.


> @@ +48,5 @@
> > +protected:
> > +  ~StreamListener() { }
> > +
> > +private:
> > +  OutputStreamDriver* mDriver;
> 
> Comment as to why a raw ptr is safe here, especially thread-safety

Done.


> @@ +53,5 @@
> > +  nsRefPtr<SourceMediaStream> mSourceStream;
> > +};
> > +
> > +OutputStreamDriver::OutputStreamDriver(DOMCanvasCaptureMediaStream* aDOMStream,
> > +                   const TrackID& aTrackId)
> 
> indent

Done.


> @@ +69,5 @@
> > +OutputStreamDriver::~OutputStreamDriver()
> > +{
> > +  if (mStreamListener) {
> > +    mStreamListener->Forget();
> > +  }
> 
> Can the mStreamListener get deleted before the next NotifyPull()?  I presume
> no (that MSG is holding a ref)

Indeed. This is the member holding the listeners on MediaStream:
> nsTArray<nsRefPtr<MediaStreamListener> > mListeners;

I'll add a comment though.


> @@ +178,5 @@
> > +  {
> > +    MOZ_ASSERT(DOMStream());
> > +    MOZ_ASSERT(NS_IsMainThread());
> > +
> > +    // mDOMStream can't be killed while we're on main thread
> 
> I presume DOMStream()->Canvas() can't either

Right.


> ::: dom/media/DOMCanvasCaptureMediaStream.h
> @@ +60,5 @@
> > +
> > +private:
> > +  DOMCanvasCaptureMediaStream* mDOMStream;
> > +  nsRefPtr<layers::Image> mImage;
> > +  Mutex mMutex;
> 
> Indicate with comments and blocking/order which things are protected by the
> mutex

I removed this mutex per my comment above.
Comment on attachment 8583764 [details] [diff] [review]
Part 1. Implement WebIDL for HTMLCanvasElement::CaptureStream

Olli, if you could have look at this I'd be thrilled. Spec draft is here: https://w3c.github.io/mediacapture-fromelement/index.html
Attachment #8583764 - Flags: review?(bugs)
This includes fixes per comment 42 and comment 44.

Randell, could you just check that I'm doing the correct thing wrt. thread safety? Also see comment 44 on that.

Jeff, I wiped WebGLContext::GetInternalImage in favor of a simple MOZ_CRASH while awaiting a patch from you. You can just put a patch on top of mine on this bug and I'll land them together. When do you think you'll have time to look at this?
Attachment #8583766 - Attachment is obsolete: true
Attachment #8585304 - Flags: review?(rjesup)
Attachment #8585304 - Flags: review?(jgilbert)
Comment on attachment 8585304 [details] [diff] [review]
Part 2. Implement HTMLCanvasElement::CaptureStream

Review of attachment 8585304 [details] [diff] [review]:
-----------------------------------------------------------------

>I was looking into NS_INLINE_DECL_THREADSAFE_REFCOUNTING a bit when going through this. Is my understanding correct that we need to guard mDriver because it's a raw pointer, and that we don't need to guard OutputStreamDriver::Image because it has threadsafe refcounting and is a RefPtr?

I haven't looked at the Image bit yet; it may be fine.  The important part is that for TSAN, you must not read a value written on another thread without a lock or other c++ memory barrier.  C++ is allowed to reuse your storage until the next point where single-threaded C++ could be tripped up by it.

::: dom/media/DOMCanvasCaptureMediaStream.cpp
@@ +50,5 @@
> +      driver = mDriver;
> +    }
> +
> +    if (driver) {
> +      driver->NotifyPull(aDesiredTime);

NotifyPull must be quick (or fairly so, since blocking it will block MSG and audio), and blocking MainThread for a short while during NotifyPull (waiting to null mDriver) should be fine.  Put the NotifyPull in the lock section, and avoid the temporary refcount bump.  Also avoids accidentally releasing it on a different thread (though that may be fine).

@@ +191,5 @@
> +
> +  nsresult
> +  TakeSnapshot()
> +  {
> +    // mDOMStream and it's mCanvas member can't be killed while we're on main thread

super-minor nit: "its"
Comment on attachment 8585304 [details] [diff] [review]
Part 2. Implement HTMLCanvasElement::CaptureStream

Review of attachment 8585304 [details] [diff] [review]:
-----------------------------------------------------------------

r- only on the locking issue; r+ if the locking is re-introduced.  (Note: I'm on PTO until the 6th, so for a timely review assign to jib or bwc or cpearce or someone else (this is generic XPCOM/threading stuff, so no special media knowledge needed).

::: dom/media/DOMCanvasCaptureMediaStream.cpp
@@ +139,5 @@
> +
> +  // Copy the reference of mImage here to ensure we don't mix it up when a new
> +  // image gets set on the main thread and we're on the MediaStreamGraph thread.
> +  // Image has threadsafe refcounting.
> +  nsRefPtr<Image> image = mImage;

I'm afraid a lock is needed here too.  Threadsafe refcounting means it's safe to increment/decrement on threads other than the original one; it doesn't mean you're safe against another thread nulling out the last nsRefPtr and causing the object to be destroyed before you can try to AddRef it, causing a UAF.  (operator= is assign_with_AddRef(aRhs.mRawPtr) -- note that it grabs the RawPtr, tests it for null, and if not null it calls AddRef() on it.  However, between when the pointer was grabbed, and when it's dereferenced to call AddRef, another thread could have nulled the ptr and freed the memory -> boom.

Also, even if you knew the object couldn't be freed out from under you, accessing the mRawPtr from thread A when thread B may write to mRawPtr is likely a TSAN violation.

See https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
and
https://bugzilla.mozilla.org/show_bug.cgi?id=1067414#c3 and following convesation
Attachment #8585304 - Flags: review?(rjesup) → review-
Comment on attachment 8583764 [details] [diff] [review]
Part 1. Implement WebIDL for HTMLCanvasElement::CaptureStream

>+++ b/dom/media/DOMCanvasCaptureMediaStream.h
>@@ -0,0 +1,50 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_dom_DOMCanvasCaptureMediaStream_h_
>+#define mozilla_dom_DOMCanvasCaptureMediaStream_h_
So this hints the class is in mozilla::dom namespace, but it isn't.
Could you call the .h/.cpp files CanvasCaptureMediaStream,* 
and then put the class to mozilla::dom:: namespace, and you don't need 
the changes to dom/bindings/Bindings.conf

>+    {name: "CanvasCaptureMediaStream", pref: "canvas.capturestream.enabled"},
This hints there is a pref controlling the interface...

>+
>+interface CanvasCaptureMediaStream : MediaStream {
>+    readonly attribute HTMLCanvasElement canvas;
>+    void requestFrame ();
>+};
But I don't see the interface being controlled by it.
So, please add Pref="canvas.capturestream.enabled" for the interface itself
Attachment #8583764 - Flags: review?(bugs) → review+
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #46)
> Jeff, I wiped WebGLContext::GetInternalImage in favor of a simple MOZ_CRASH
> while awaiting a patch from you. You can just put a patch on top of mine on
> this bug and I'll land them together. When do you think you'll have time to
> look at this?

OK, I'll see what I can do this week.

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #38)
> Now testing that we cannot captureStream() on a 2d canvas that is not
> origin-clean, and that captured streams from a webgl canvas draw properly in
> the various modes (framerates 10 and 0, with requestFrame(), for now).
> 
> Jeff, is there any way to make a webgl canvas write only? As I understood
> things it is illegal to upload cross origing textures in the first place, so
> it will never reach a write-only state.

No, webgl never becomes write-only, though we do have context loss to deal with.
Comment on attachment 8585304 [details] [diff] [review]
Part 2. Implement HTMLCanvasElement::CaptureStream

Review of attachment 8585304 [details] [diff] [review]:
-----------------------------------------------------------------

WebGL stubs LGTM. Did you want anything else from me here for now?
Attachment #8585304 - Flags: review?(jgilbert) → review+
(In reply to Randell Jesup|PTO until Apr 6 [:jesup] from comment #48)
> Comment on attachment 8585304 [details] [diff] [review]
> Part 2. Implement HTMLCanvasElement::CaptureStream
> 
> Review of attachment 8585304 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- only on the locking issue; r+ if the locking is re-introduced.  (Note:
> I'm on PTO until the 6th, so for a timely review assign to jib or bwc or
> cpearce or someone else (this is generic XPCOM/threading stuff, so no
> special media knowledge needed).
Right, I'll fix that. Thanks for the handy info!
(In reply to Olli Pettay [:smaug] from comment #49)
> Comment on attachment 8583764 [details] [diff] [review]
> Part 1. Implement WebIDL for HTMLCanvasElement::CaptureStream
> 
> >+++ b/dom/media/DOMCanvasCaptureMediaStream.h
> >@@ -0,0 +1,50 @@
> >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/
> >+/* This Source Code Form is subject to the terms of the Mozilla Public
> >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >+
> >+#ifndef mozilla_dom_DOMCanvasCaptureMediaStream_h_
> >+#define mozilla_dom_DOMCanvasCaptureMediaStream_h_
> So this hints the class is in mozilla::dom namespace, but it isn't.
> Could you call the .h/.cpp files CanvasCaptureMediaStream,* 
> and then put the class to mozilla::dom:: namespace, and you don't need 
> the changes to dom/bindings/Bindings.conf

Sure, I'll fix that. I merely followed the naming and namespace of DOMMediaStream and friends (DOMLocalMediaStream, DOMAudioNodeMediaStream). Not sure why it is the way it is.


> >+    {name: "CanvasCaptureMediaStream", pref: "canvas.capturestream.enabled"},
> This hints there is a pref controlling the interface...
> 
> >+
> >+interface CanvasCaptureMediaStream : MediaStream {
> >+    readonly attribute HTMLCanvasElement canvas;
> >+    void requestFrame ();
> >+};
> But I don't see the interface being controlled by it.
> So, please add Pref="canvas.capturestream.enabled" for the interface itself

Added!
(In reply to Jeff Gilbert [:jgilbert] from comment #51)
> Comment on attachment 8585304 [details] [diff] [review]
> Part 2. Implement HTMLCanvasElement::CaptureStream
> 
> Review of attachment 8585304 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> WebGL stubs LGTM. Did you want anything else from me here for now?

No, just your GetInternalImage() implementation :)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #53) 
> Sure, I'll fix that. I merely followed the naming and namespace of
> DOMMediaStream and friends (DOMLocalMediaStream, DOMAudioNodeMediaStream).
> Not sure why it is the way it is.
Maybe historical reasons? Stuff used to have DOM prefix more often before the new bindings, which
default to mozilla::dom namespace.
Fixes per comment 49. Carrying forward r=mt,smaug.
Attachment #8583764 - Attachment is obsolete: true
Attachment #8589055 - Flags: review+
Fixes per comment 47 and comment 48, carrying forward r=jgilbert for the webgl stubs.

r? jesup for the locking stuff in comment 48.

r? gwright for CanvasRenderingContext2D::GetInternalImage.

r? mt for spec compliance as followup to the tests (coming soon) as requested in comment 33.
Attachment #8585304 - Attachment is obsolete: true
Attachment #8589066 - Flags: review?(rjesup)
Attachment #8589066 - Flags: review?(martin.thomson)
Attachment #8589066 - Flags: review?(gwright)
This set of tests should be complete. WebGL tests will fail for now due to the missing implementation of GetInternalImage.

r? mt for spec compliance

r? jgilbert for webgl tests (.../webgl-mochitest/test_capture.html and .../test_peerConnection_captureStream_canvas_webgl.html)

r? gwright for canvas2d tests (test_capture.html, crossorigin tests and test_peerConnection_captureStream_canvas_2d.html)

r? jesup for peerConnection tests
Attachment #8584418 - Attachment is obsolete: true
Attachment #8589072 - Flags: review?(rjesup)
Attachment #8589072 - Flags: review?(martin.thomson)
Attachment #8589072 - Flags: review?(jgilbert)
Attachment #8589072 - Flags: review?(gwright)
Comment on attachment 8583767 [details] [diff] [review]
Part 4. Do not use TextureClientX11 on background threads for now

Not needed anymore - fixed by bug 1147777.
Attachment #8583767 - Attachment is obsolete: true
Attachment #8589066 - Attachment description: Part 2. Implement HTMLCanvasElement::CaptureStream → Part 2. Implement HTMLCanvasElement::CaptureStream (carries r=jgilbert)
Comment on attachment 8589066 [details] [diff] [review]
Part 2. Implement HTMLCanvasElement::CaptureStream (carries r=jgilbert)

Review of attachment 8589066 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/CanvasCaptureMediaStream.cpp
@@ +57,5 @@
> +protected:
> +  ~StreamListener() { }
> +
> +private:
> +  Mutex mMutex;

move down (or move mSourceStream up)

@@ +148,5 @@
> +{
> +  StreamTime delta = aDesiredTime - mSourceStream->GetEndOfAppendedData(mTrackId);
> +  if (delta > 0) {
> +    // nullptr images are allowed
> +    AppendToTrack(delta);

I presume (it really should be documented in MediaStreamGraph.h!) that a nullptr video image for a segment simply extends the duration, much as passing the same image in extends the duration.

I think someone should double-check sometime that we're not wasting cycles here due to lack of optimizations for the same-image or null-image cases in VideoSegment/MediaSegment/etc (also when forcing black).  Please file a bug to investigate this that we can look at later; cc padenot and myself and roc

@@ +220,5 @@
> +
> +protected:
> +  virtual ~TimerDriver()
> +  {
> +  }

nit: virtual ~TimerDriver() {} on one line is more common.  And arguable.

@@ +236,5 @@
> +    mTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +    if (!mTimer) {
> +      return;
> +    }
> +    mTimer->InitWithCallback(this, int(1000 / mFPS), nsITimer::TYPE_REPEATING_SLACK);

I have a concern about mFPS for two reasons - one is that we're doing exact comparison with 0.0 (admittedly semi-reasonable in this case, but really being used as a flag).  Second we're dividing by mFPS, which could yield silly FPS values if not capped (as we do below) causing serious perf/DoS issues by monopolizing MainThread, especially if the programmer naively calculates or allows users to set the value.

There may be some cases where a high rate is reasonable (>60fps when generating a local recording of a rendering), but we need to be careful about overloading MainThread.  So, that said, we should consider if values >> 30fps are reasonable, and if not what the limit should be (and if it should be in some way adaptive).  I doubt 1000fps is reasonable, for example.  Down below we cap it to 60fps, which likely is reasonable for live/WebRTC use, but might not be optimal for some recording uses.  However: 60fps isn't a bad place to start.

I suggest reaching out to Vlad and/or other people who interact with WebGL/games/etc, but land as-is

::: dom/media/CanvasCaptureMediaStream.h
@@ +60,5 @@
> +  // This is a raw pointer to avoid a reference cycle between OutputStreamDriver
> +  // and CanvasCaptureMediaStream. ForgetDOMStream() will be called by
> +  // ~CanvasCaptureMediaStream() to make sure we don't do anything illegal.
> +  CanvasCaptureMediaStream* mDOMStream;
> +  Mutex mMutex;

move this to right before the items protected; we try to keep them in a block with the mutex.  May require updating the initializers for the constructor.
Attachment #8589066 - Attachment description: Part 2. Implement HTMLCanvasElement::CaptureStream (carries r=jgilbert) → Part 2. Implement HTMLCanvasElement::CaptureStream
Attachment #8589066 - Flags: review?(rjesup) → review+
Attachment #8589066 - Attachment description: Part 2. Implement HTMLCanvasElement::CaptureStream → Part 2. Implement HTMLCanvasElement::CaptureStream (carries r=jgilbert)
Comment on attachment 8589072 [details] [diff] [review]
Part 3. Add tests for HTMLCanvasElement::CaptureStream

Review of attachment 8589072 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_2d.html
@@ +29,5 @@
> +    info("Checking remote video element for color data " + color);
> +    v.ontimeupdate = function() {
> +      ctx.drawImage(v, 0, 0, c.width, c.height);
> +      var out = ctx.getImageData(0, 0, 1, 1);
> +      if (color.every((val, i) => Math.abs(val - out.data[i]) < 5)) {

please comment on this - this is basically "I hope the video codec generates something close to the source color"...
5 may be a overly-restrictive value - basically if we change the source and the destination changes (and is anything at all like the source), I'd count that as passed; otherwise we need to use actual images and some sort of PSNR image-comparitor.  I suggest widening the acceptance range a lot.  Ditto for the other test.
Attachment #8589072 - Flags: review?(rjesup) → review+
Comment on attachment 8589072 [details] [diff] [review]
Part 3. Add tests for HTMLCanvasElement::CaptureStream

Review of attachment 8589072 [details] [diff] [review]:
-----------------------------------------------------------------

I think that I'd like to see this just once more.

BTW, Reviewboard would have made this easier to follow where you had to re-indent a bunch of text.

::: dom/canvas/test/crossorigin/test_video_crossorigin.html
@@ +46,5 @@
> +    ok(true, "drawImage '" + v.src + "' then captureStream with crossOrigin='" + v.crossOrigin + "' worked");
> +  } catch(error) {
> +    ok(!v.crossOrigin && error.name === "SecurityError", "drawImage '" + v.src + "' then captureStream with crossOrigin='" + v.crossOrigin + "' failed");
> +    v.tainted = true;
> +  }

I think that you should try to reduce code duplication with the block above somehow.  Also below.

::: dom/canvas/test/test_capture.html
@@ +175,5 @@
> +
> +  ok(testVideoPixel(vauto, [0, 0, 0, 0]), "Should not be drawn to before stable state");
> +  ok(testVideoPixel(vrate, [0, 0, 0, 0]), "Should not be drawn to before stable state");
> +  ok(testVideoPixel(vmanual, [0, 0, 0, 0]), "Should not be drawn to before stable state");
> +  step1();

nit: I'd not have the steps know about each other and instead do:

Promise.resolve()
  .then(step1)
  .then(step2)
  .then(step3)
  .then(finish);

I'd also call each step something less generic: checkInitial, redrawAndCheck, notOriginClean, or something (my names aren't necessarily the best either).

::: dom/canvas/test/webgl-mochitest/test_capture.html
@@ +51,5 @@
> +  /* Requests a frame from the stream on this video element */
> +  function requestFrame(video) {
> +    info("Requesting frame from " + video.id);
> +    video.mozSrcObject.requestFrame();
> +    return Promise.resolve();

No need to return Promise.resolve() here.

@@ +55,5 @@
> +    return Promise.resolve();
> +  }
> +
> +  /* Tests the top left pixel of |video| against |refData| (format [R,G,B,A]) */
> +  function testVideoPixel(video, refData) {

Duplicates code in the other test.

@@ +89,5 @@
> +    info("START drawArrays");
> +    gl.clearColor(1.0, 0.0, 0.0, 1.0);
> +    gl.clear(gl.COLOR_BUFFER_BIT);
> +    return Promise.resolve()
> +      .then(() => waitForVideoPixel(vauto, [255, 0, 0, 255], "should become red automatically"))

Should this be waitForVideoPixel or testVideoPixel?  What are the ordering guarantees?

@@ +102,5 @@
> +    return Promise.resolve()
> +      .then(() => waitForVideoPixel(vauto, [0, 255, 0, 255], "should become green automatically"))
> +      .then(() => waitForVideoPixel(vrate, [0, 255, 0, 255], "should become green automatically"))
> +      .then(() => waitForVideoPixel(vmanual, [255, 0, 0, 255], "should still be red"))
> +      .then(() => requestFrame(vmanual))

I notice that you don't have a test step that does a draw immediately preceding requestFrame().  All your tests have some delay between the draw and the request.  

I think that I'd like to see that so that we can understand the ordering guarantees there.  Both here and in the canvas.

::: dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_2d.html
@@ +10,5 @@
> +  bug: "1032848",
> +  title: "Canvas(2D)::CaptureStream as video-only input to peerconnection"
> +});
> +
> +var drawColor = function DRAW_COLOR(color, test) {

Just use the all caps name, no need for an extra variable.

@@ +16,5 @@
> +  var ctx = c.getContext('2d');
> +  color[3] = color[3] / 255.0; // Convert to double in range [0,1]
> +  ctx.fillStyle = "rgba(" + color.join(',') + ")";
> +  ctx.fillRect(0, 0, 5, 5);
> +  return Promise.resolve();

You don't need to return a promise here.

@@ +20,5 @@
> +  return Promise.resolve();
> +};
> +
> +var checkVideoOutput = function CHECK_VIDEO_OUTPUT(color, test) {
> +  return new Promise(function(resolve) {

s/function(resolve)/resolve =>/

@@ +26,5 @@
> +    var c = document.createElement('canvas');
> +    c.width = c.height = 10;
> +    var ctx = c.getContext('2d');
> +    info("Checking remote video element for color data " + color);
> +    v.ontimeupdate = function() {

() =>

@@ +29,5 @@
> +    info("Checking remote video element for color data " + color);
> +    v.ontimeupdate = function() {
> +      ctx.drawImage(v, 0, 0, c.width, c.height);
> +      var out = ctx.getImageData(0, 0, 1, 1);
> +      if (color.every((val, i) => Math.abs(val - out.data[i]) < 5)) {

I agree with Randall here.  As long as you use strongly differentiated values (i.e., a long way from black or white), this should be fine.

@@ +54,5 @@
> +  test.chain.replace("PC_LOCAL_GUM", [
> +    function PC_LOCAL_CANVAS_SOURCE(test) {
> +      var stream = canvas.captureStream(10);
> +      test.pcLocal.attachMedia(stream, 'video', 'local');
> +      return Promise.resolve();

Remove the return.

@@ +68,5 @@
> +
> +var prefs = [
> +  [ "canvas.capturestream.enabled", true ],
> +];
> +SpecialPowers.pushPrefEnv({ "set" : prefs }, beginTest);

Please just add this pref to the set of prefs in head.js.  This way of setting preferences is brittle, because SpecialPowers might not be set when you execute this code (I'm actually a little surprised that this worked at all).

https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/head.js#132

::: dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_webgl.html
@@ +36,5 @@
> +  info("Drew color " + color.join(','));
> +  return Promise.resolve();
> +};
> +
> +var checkVideoOutput = function CHECK_VIDEO_OUTPUT(color, test) {

This duplicates code in the other test.

@@ +116,5 @@
> +    function PC_LOCAL_CANVAS_SOURCE(test) {
> +      test.pcLocal.canvasStream = canvas.captureStream(0.0);
> +      is(test.pcLocal.canvasStream.canvas, canvas, "Canvas attribute is correct");
> +      test.pcLocal.attachMedia(test.pcLocal.canvasStream, 'video', 'local');
> +      return Promise.resolve();

Remove.

@@ +123,5 @@
> +  test.chain.append([
> +    checkVideoOutput.bind(null, [0, 0, 255, 255]),
> +    drawColor.bind(null, [255, 0, 0, 255]),
> +    REQUEST_FRAME,
> +    checkVideoOutput.bind(null, [255, 0, 0, 255]),

Adding these to the test chain will get some very strange log messages.  Why not roll these calls into one or more functions with consistent names.

test.chain.append([
  function VIDEO_IS_BLUE() {
    return checkVideoOutput([0, 0, 255, 255]);
  },
  function DRAW_RED(t) {
    drawColor([255, 0, 0, 255]);
    t.pcLocal.canvasStream.requestFrame();
  },
  function VIDEO_IS_RED() {
    return checkVideoOutput([255, 0, 0, 255]);
  }
]);
Attachment #8589072 - Flags: review?(martin.thomson)
Comment on attachment 8589066 [details] [diff] [review]
Part 2. Implement HTMLCanvasElement::CaptureStream (carries r=jgilbert)

Review of attachment 8589066 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/CanvasCaptureMediaStream.cpp
@@ +292,5 @@
>                                 const TrackID& aTrackId)
>  {
>    if (!aFPS.WasPassed()) {
> +    // TODO: Implement a real AutoDriver. We do a 30FPS TimerDriver for now.
> +    mOutputStreamDriver = new TimerDriver(this, 30.0, aTrackId);

A bug number is customary in these cases.
Attachment #8589066 - Flags: review?(martin.thomson) → review+
Blocks: 1152298
Blocks: 1152317
Blocks: 1152328
(In reply to Randell Jesup [:jesup] from comment #60)
> Comment on attachment 8589066 [details] [diff] [review]
> Part 2. Implement HTMLCanvasElement::CaptureStream (carries r=jgilbert)
> 
> Review of attachment 8589066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/CanvasCaptureMediaStream.cpp
> @@ +57,5 @@
> > +protected:
> > +  ~StreamListener() { }
> > +
> > +private:
> > +  Mutex mMutex;
> 
> move down (or move mSourceStream up)
Fixed.

> @@ +148,5 @@
> > +{
> > +  StreamTime delta = aDesiredTime - mSourceStream->GetEndOfAppendedData(mTrackId);
> > +  if (delta > 0) {
> > +    // nullptr images are allowed
> > +    AppendToTrack(delta);
> 
> I presume (it really should be documented in MediaStreamGraph.h!) that a
> nullptr video image for a segment simply extends the duration, much as
> passing the same image in extends the duration.
I'm only passing null images before we have gotten the first image from the canvas; and after that always passing in the old image - until we get a new one.

> I think someone should double-check sometime that we're not wasting cycles
> here due to lack of optimizations for the same-image or null-image cases in
> VideoSegment/MediaSegment/etc (also when forcing black).  Please file a bug
> to investigate this that we can look at later; cc padenot and myself and roc
Filed bug 1152317.

> @@ +220,5 @@
> > +
> > +protected:
> > +  virtual ~TimerDriver()
> > +  {
> > +  }
> 
> nit: virtual ~TimerDriver() {} on one line is more common.  And arguable.
Fixed.

> @@ +236,5 @@
> > +    mTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
> > +    if (!mTimer) {
> > +      return;
> > +    }
> > +    mTimer->InitWithCallback(this, int(1000 / mFPS), nsITimer::TYPE_REPEATING_SLACK);
> 
> I have a concern about mFPS for two reasons - one is that we're doing exact
> comparison with 0.0 (admittedly semi-reasonable in this case, but really
> being used as a flag).  Second we're dividing by mFPS, which could yield
> silly FPS values if not capped (as we do below) causing serious perf/DoS
> issues by monopolizing MainThread, especially if the programmer naively
> calculates or allows users to set the value.
> 
> There may be some cases where a high rate is reasonable (>60fps when
> generating a local recording of a rendering), but we need to be careful
> about overloading MainThread.  So, that said, we should consider if values
> >> 30fps are reasonable, and if not what the limit should be (and if it
> should be in some way adaptive).  I doubt 1000fps is reasonable, for
> example.  Down below we cap it to 60fps, which likely is reasonable for
> live/WebRTC use, but might not be optimal for some recording uses.  However:
> 60fps isn't a bad place to start.
> 
> I suggest reaching out to Vlad and/or other people who interact with
> WebGL/games/etc, but land as-is
Filed bug 1152328.

> ::: dom/media/CanvasCaptureMediaStream.h
> @@ +60,5 @@
> > +  // This is a raw pointer to avoid a reference cycle between OutputStreamDriver
> > +  // and CanvasCaptureMediaStream. ForgetDOMStream() will be called by
> > +  // ~CanvasCaptureMediaStream() to make sure we don't do anything illegal.
> > +  CanvasCaptureMediaStream* mDOMStream;
> > +  Mutex mMutex;
> 
> move this to right before the items protected; we try to keep them in a
> block with the mutex.  May require updating the initializers for the
> constructor.
Fixed.
(In reply to Martin Thomson [:mt] from comment #63)
> Comment on attachment 8589066 [details] [diff] [review]
> Part 2. Implement HTMLCanvasElement::CaptureStream (carries r=jgilbert)
> 
> Review of attachment 8589066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/CanvasCaptureMediaStream.cpp
> @@ +292,5 @@
> >                                 const TrackID& aTrackId)
> >  {
> >    if (!aFPS.WasPassed()) {
> > +    // TODO: Implement a real AutoDriver. We do a 30FPS TimerDriver for now.
> > +    mOutputStreamDriver = new TimerDriver(this, 30.0, aTrackId);
> 
> A bug number is customary in these cases.
Fixed; filed bug 1152298.
(In reply to Randell Jesup [:jesup] from comment #61)
> Comment on attachment 8589072 [details] [diff] [review]
> Part 3. Add tests for HTMLCanvasElement::CaptureStream
> 
> Review of attachment 8589072 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_2d.html
> @@ +29,5 @@
> > +    info("Checking remote video element for color data " + color);
> > +    v.ontimeupdate = function() {
> > +      ctx.drawImage(v, 0, 0, c.width, c.height);
> > +      var out = ctx.getImageData(0, 0, 1, 1);
> > +      if (color.every((val, i) => Math.abs(val - out.data[i]) < 5)) {
> 
> please comment on this - this is basically "I hope the video codec generates
> something close to the source color"...
> 5 may be a overly-restrictive value - basically if we change the source and
> the destination changes (and is anything at all like the source), I'd count
> that as passed; otherwise we need to use actual images and some sort of PSNR
> image-comparitor.  I suggest widening the acceptance range a lot.  Ditto for
> the other test.

Added comment and changed threshold to 128 for both tests.
Comment on attachment 8589066 [details] [diff] [review]
Part 2. Implement HTMLCanvasElement::CaptureStream (carries r=jgilbert)

Review of attachment 8589066 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/CanvasRenderingContext2D.h
@@ +464,5 @@
> +    RefPtr<layers::CairoImage> image = new layers::CairoImage();
> +    layers::CairoImage::Data imageData;
> +    imageData.mSize = data->GetSize();
> +    imageData.mSourceSurface = data;
> +    image->SetData(imageData);

This can't be called off the main thread. I assume that's not a problem now, but will it be in the future once we head towards off main thread canvas?
Attachment #8589066 - Flags: review?(gwright) → review+
Comment on attachment 8589072 [details] [diff] [review]
Part 3. Add tests for HTMLCanvasElement::CaptureStream

Not really familiar enough with our tests to give a good review here. It looks like :mt already has a decent handle on it though.
Attachment #8589072 - Flags: review?(gwright)
> > @@ +148,5 @@
> > > +{
> > > +  StreamTime delta = aDesiredTime - mSourceStream->GetEndOfAppendedData(mTrackId);
> > > +  if (delta > 0) {
> > > +    // nullptr images are allowed
> > > +    AppendToTrack(delta);
> > 
> > I presume (it really should be documented in MediaStreamGraph.h!) that a
> > nullptr video image for a segment simply extends the duration, much as
> > passing the same image in extends the duration.
> I'm only passing null images before we have gotten the first image from the
> canvas; and after that always passing in the old image - until we get a new
> one.

In AppendToTrack(delta):
 
  segment.AppendFrame(mImage.forget(), aDuration, size);

This appears to pass each image once, then null
Excellent review. It took a while to do the overhaul but it looks much better now. I'll put up the patch for an extra round soon.

(In reply to Martin Thomson [:mt] from comment #62)
> Comment on attachment 8589072 [details] [diff] [review]
> Part 3. Add tests for HTMLCanvasElement::CaptureStream
> 
> Review of attachment 8589072 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think that I'd like to see this just once more.
> 
> BTW, Reviewboard would have made this easier to follow where you had to
> re-indent a bunch of text.
Yeah I know, sorry about the extra work. I'm doing all my work in git and last time I checked there was no good tooling in place for pushing to Reviewboard with git.

> ::: dom/canvas/test/crossorigin/test_video_crossorigin.html
> @@ +46,5 @@
> > +    ok(true, "drawImage '" + v.src + "' then captureStream with crossOrigin='" + v.crossOrigin + "' worked");
> > +  } catch(error) {
> > +    ok(!v.crossOrigin && error.name === "SecurityError", "drawImage '" + v.src + "' then captureStream with crossOrigin='" + v.crossOrigin + "' failed");
> > +    v.tainted = true;
> > +  }
> 
> I think that you should try to reduce code duplication with the block above
> somehow.  Also below.
Will do.

> ::: dom/canvas/test/test_capture.html
> @@ +175,5 @@
> > +
> > +  ok(testVideoPixel(vauto, [0, 0, 0, 0]), "Should not be drawn to before stable state");
> > +  ok(testVideoPixel(vrate, [0, 0, 0, 0]), "Should not be drawn to before stable state");
> > +  ok(testVideoPixel(vmanual, [0, 0, 0, 0]), "Should not be drawn to before stable state");
> > +  step1();
> 
> nit: I'd not have the steps know about each other and instead do:
> 
> Promise.resolve()
>   .then(step1)
>   .then(step2)
>   .then(step3)
>   .then(finish);
Done!

> I'd also call each step something less generic: checkInitial,
> redrawAndCheck, notOriginClean, or something (my names aren't necessarily
> the best either).
Done. We now have checkDrawColorInitialRed, checkDrawColorGreen and checkDrawNotCleanRedImg.

> ::: dom/canvas/test/webgl-mochitest/test_capture.html
> @@ +51,5 @@
> > +  /* Requests a frame from the stream on this video element */
> > +  function requestFrame(video) {
> > +    info("Requesting frame from " + video.id);
> > +    video.mozSrcObject.requestFrame();
> > +    return Promise.resolve();
> 
> No need to return Promise.resolve() here.
Fixed.

> @@ +55,5 @@
> > +    return Promise.resolve();
> > +  }
> > +
> > +  /* Tests the top left pixel of |video| against |refData| (format [R,G,B,A]) */
> > +  function testVideoPixel(video, refData) {
> 
> Duplicates code in the other test.
Broke them out to captureStream_common.js.

> @@ +89,5 @@
> > +    info("START drawArrays");
> > +    gl.clearColor(1.0, 0.0, 0.0, 1.0);
> > +    gl.clear(gl.COLOR_BUFFER_BIT);
> > +    return Promise.resolve()
> > +      .then(() => waitForVideoPixel(vauto, [255, 0, 0, 255], "should become red automatically"))
> 
> Should this be waitForVideoPixel or testVideoPixel?  What are the ordering
> guarantees?
testVideoPixel will test the color of the video element right away, but it will take some time for the streams to propagate the new frame after the gl.clear(). (first we need a stable state, then the timer has to fire, etc.)
waitForVideoPixel will return a promise that resolves when the pixel color matches, or the test will time out.
Due to the frame delay we have to use waitForVideoPixel.

> @@ +102,5 @@
> > +    return Promise.resolve()
> > +      .then(() => waitForVideoPixel(vauto, [0, 255, 0, 255], "should become green automatically"))
> > +      .then(() => waitForVideoPixel(vrate, [0, 255, 0, 255], "should become green automatically"))
> > +      .then(() => waitForVideoPixel(vmanual, [255, 0, 0, 255], "should still be red"))
> > +      .then(() => requestFrame(vmanual))
> 
> I notice that you don't have a test step that does a draw immediately
> preceding requestFrame().  All your tests have some delay between the draw
> and the request.  
> 
> I think that I'd like to see that so that we can understand the ordering
> guarantees there.  Both here and in the canvas.
Agree. I added a step "checkRequestFrameOrderGuarantee" which essentially does:
> drawColor(red)
> requestFrame()
> drawColor(green)
and checks that we eventually get a red frame, and after that don't get a green one.

> :::
> dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_2d.html
> @@ +10,5 @@
> > +  bug: "1032848",
> > +  title: "Canvas(2D)::CaptureStream as video-only input to peerconnection"
> > +});
> > +
> > +var drawColor = function DRAW_COLOR(color, test) {
> 
> Just use the all caps name, no need for an extra variable.
Done.

> @@ +16,5 @@
> > +  var ctx = c.getContext('2d');
> > +  color[3] = color[3] / 255.0; // Convert to double in range [0,1]
> > +  ctx.fillStyle = "rgba(" + color.join(',') + ")";
> > +  ctx.fillRect(0, 0, 5, 5);
> > +  return Promise.resolve();
> 
> You don't need to return a promise here.
Done.

> @@ +20,5 @@
> > +  return Promise.resolve();
> > +};
> > +
> > +var checkVideoOutput = function CHECK_VIDEO_OUTPUT(color, test) {
> > +  return new Promise(function(resolve) {
> 
> s/function(resolve)/resolve =>/
Done.

> @@ +26,5 @@
> > +    var c = document.createElement('canvas');
> > +    c.width = c.height = 10;
> > +    var ctx = c.getContext('2d');
> > +    info("Checking remote video element for color data " + color);
> > +    v.ontimeupdate = function() {
> 
> () =>
Done.

> @@ +29,5 @@
> > +    info("Checking remote video element for color data " + color);
> > +    v.ontimeupdate = function() {
> > +      ctx.drawImage(v, 0, 0, c.width, c.height);
> > +      var out = ctx.getImageData(0, 0, 1, 1);
> > +      if (color.every((val, i) => Math.abs(val - out.data[i]) < 5)) {
> 
> I agree with Randall here.  As long as you use strongly differentiated
> values (i.e., a long way from black or white), this should be fine.
Done, per comment 66.

> @@ +54,5 @@
> > +  test.chain.replace("PC_LOCAL_GUM", [
> > +    function PC_LOCAL_CANVAS_SOURCE(test) {
> > +      var stream = canvas.captureStream(10);
> > +      test.pcLocal.attachMedia(stream, 'video', 'local');
> > +      return Promise.resolve();
> 
> Remove the return.
Done.

> @@ +68,5 @@
> > +
> > +var prefs = [
> > +  [ "canvas.capturestream.enabled", true ],
> > +];
> > +SpecialPowers.pushPrefEnv({ "set" : prefs }, beginTest);
> 
> Please just add this pref to the set of prefs in head.js.  This way of
> setting preferences is brittle, because SpecialPowers might not be set when
> you execute this code (I'm actually a little surprised that this worked at
> all).
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/
> head.js#132
Ah! That's how it should be done ;-)

> :::
> dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_webgl.html
> @@ +36,5 @@
> > +  info("Drew color " + color.join(','));
> > +  return Promise.resolve();
> > +};
> > +
> > +var checkVideoOutput = function CHECK_VIDEO_OUTPUT(color, test) {
> 
> This duplicates code in the other test.
Indeed. I included the new captureStream_common.js from the /canvas/ tests and used those functions instead. Much cleaner now.

> @@ +116,5 @@
> > +    function PC_LOCAL_CANVAS_SOURCE(test) {
> > +      test.pcLocal.canvasStream = canvas.captureStream(0.0);
> > +      is(test.pcLocal.canvasStream.canvas, canvas, "Canvas attribute is correct");
> > +      test.pcLocal.attachMedia(test.pcLocal.canvasStream, 'video', 'local');
> > +      return Promise.resolve();
> 
> Remove.
Done.

> @@ +123,5 @@
> > +  test.chain.append([
> > +    checkVideoOutput.bind(null, [0, 0, 255, 255]),
> > +    drawColor.bind(null, [255, 0, 0, 255]),
> > +    REQUEST_FRAME,
> > +    checkVideoOutput.bind(null, [255, 0, 0, 255]),
> 
> Adding these to the test chain will get some very strange log messages.  Why
> not roll these calls into one or more functions with consistent names.
> 
> test.chain.append([
>   function VIDEO_IS_BLUE() {
>     return checkVideoOutput([0, 0, 255, 255]);
>   },
>   function DRAW_RED(t) {
>     drawColor([255, 0, 0, 255]);
>     t.pcLocal.canvasStream.requestFrame();
>   },
>   function VIDEO_IS_RED() {
>     return checkVideoOutput([255, 0, 0, 255]);
>   }
> ]);
Excellent. Done!
(In reply to George Wright (:gw280) from comment #67)
> Comment on attachment 8589066 [details] [diff] [review]
> Part 2. Implement HTMLCanvasElement::CaptureStream (carries r=jgilbert)
> 
> Review of attachment 8589066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/CanvasRenderingContext2D.h
> @@ +464,5 @@
> > +    RefPtr<layers::CairoImage> image = new layers::CairoImage();
> > +    layers::CairoImage::Data imageData;
> > +    imageData.mSize = data->GetSize();
> > +    imageData.mSourceSurface = data;
> > +    image->SetData(imageData);
> 
> This can't be called off the main thread. I assume that's not a problem now,
> but will it be in the future once we head towards off main thread canvas?

Indeed not a problem now. I don't know anything about off main thread canvas so I can't vouch for the future, but if it is as simple as a dispatch back and forth it won't be a problem.
(In reply to Randell Jesup [:jesup] from comment #69)
> In AppendToTrack(delta):
>  
>   segment.AppendFrame(mImage.forget(), aDuration, size);
> 
> This appears to pass each image once, then null

Oh, you're right! I think I changed to this while eyeing through code mid-review - apparently not knowing what I was doing :)

Fixed to take a copy of the mImage ref and then forget()ing that instead.
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #70)
> > BTW, Reviewboard would have made this easier to follow where you had to
> > re-indent a bunch of text.
> Yeah I know, sorry about the extra work. I'm doing all my work in git and
> last time I checked there was no good tooling in place for pushing to
> Reviewboard with git.

http://glandium.org/blog/?p=3405 might work for you.  I haven't tried it yet, but plan to for exactly this reason.

I currently use a low-tech option.  It's a script that runs git format-patch and then hg import.  I can share it if you feel like slumming it.
/r/6907 - Bug 1032848 - Part 1. Implement WebIDL for HTMLCanvasElement::CaptureStream. r=smaug,mt
/r/6909 - Bug 1032848 - Part 2. Implement HTMLCanvasElement::CaptureStream. r=mt,jesup,jgilbert,gwright
/r/6911 - Bug 1032848 - Part 3. Add tests for HTMLCanvasElement::CaptureStream. r=mt,jgilbert,jesup

Pull down these commits:

hg pull -r a2269478baafa9ef6a1e0a58b9390c7fe7245b24 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8590988 - Flags: review?(martin.thomson)
Attachment #8590988 - Flags: review?(jgilbert)
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

https://reviewboard.mozilla.org/r/6905/#review5747

LGTM

::: dom/canvas/test/captureStream_common.js
(Diff revision 1)
> +  clear_2d: function(canvas) {

I think that I'd prefer to see a base class with 2d and gl specializations here.
Attachment #8590988 - Flags: review?(martin.thomson) → review+
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

/r/6907 - Bug 1032848 - Part 1. Implement WebIDL for HTMLCanvasElement::CaptureStream. r=smaug,mt
/r/6909 - Bug 1032848 - Part 2. Implement HTMLCanvasElement::CaptureStream. r=mt,jesup,jgilbert,gwright
/r/6911 - Bug 1032848 - Part 3. Add tests for HTMLCanvasElement::CaptureStream. r=mt,jgilbert,jesup

Pull down these commits:

hg pull -r 4d26b206bb522577a20720d3ca60b48c19cc9997 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8590988 - Flags: review+ → review?(martin.thomson)
Not quite sure how I got part 1 and 3 discarded - pretty sure I clicked publish on them at some point (individually, not on the parent). Doesn't seem to be a way to get them back now.
Attachment #8589055 - Attachment is obsolete: true
Attachment #8589066 - Attachment is obsolete: true
Attachment #8589072 - Attachment is obsolete: true
Attachment #8589072 - Flags: review?(jgilbert)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #77)
> Not quite sure how I got part 1 and 3 discarded - pretty sure I clicked
> publish on them at some point (individually, not on the parent). Doesn't
> seem to be a way to get them back now.

Don't worry about that: reviewboard seems to operate on the entire set of patches at once.  Trying anything else only gets it angry.
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

Sometimes RB misses this part, I can't work out why.
Attachment #8590988 - Flags: review?(martin.thomson) → review+
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

/r/6907 - Bug 1032848 - Part 1. Implement WebIDL for HTMLCanvasElement::CaptureStream. r=smaug,mt
/r/6909 - Bug 1032848 - Part 2. Implement HTMLCanvasElement::CaptureStream. r=mt,jesup,jgilbert,gwright
/r/6911 - Bug 1032848 - Part 3. Add tests for HTMLCanvasElement::CaptureStream. r=mt,jgilbert,jesup

Pull down these commits:

hg pull -r a7a670e61a45ee010283e20d68e470984ded4b47 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8590988 - Flags: review+ → review?(martin.thomson)
Sorry for the new r? mt; I'm still working out the workflow in rb ;-)
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

/r/6907 - Bug 1032848 - Part 1. Implement WebIDL for HTMLCanvasElement::CaptureStream. r=smaug,mt
/r/6909 - Bug 1032848 - Part 2. Implement HTMLCanvasElement::CaptureStream. r=mt,jesup,jgilbert,gwright
/r/6911 - Bug 1032848 - Part 3. Add tests for HTMLCanvasElement::CaptureStream. r=mt,jgilbert,jesup

Pull down these commits:

hg pull -r 70b45b98a4c4a73d939a5ee0e5eaf4cb8091eb9f https://reviewboard-hg.mozilla.org/gecko/
Attachment #8590988 - Flags: review?(martin.thomson) → review+
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

https://reviewboard.mozilla.org/r/6905/#review6523
Attachment #8590988 - Flags: review?(jgilbert) → review+
Does this work?

The canvas path uses this software readback path, so we can just cop off that.

Also, since the canvas path doesn't seem to specify, it seems like this entrypoint expects an alpha-premultiplied Image. If this is true, it should be explicitly stated on at least the WebGL side of things.
Flags: needinfo?(jgilbert)
Attachment #8598378 - Flags: feedback?(pehrsons)
Comment on attachment 8598378 [details] [diff] [review]
0001-Add-WebGLContext-GetInternalImage.patch

Review of attachment 8598378 [details] [diff] [review]:
-----------------------------------------------------------------

This requires `{preserveDrawingBuffer: true}` to work. How can we make it work without it?

Run `./mach mochitest-plain --subsuite webgl dom/canvas/test/webgl-mochitest/test_capture.html` and it'll fail because of this. The streams driven by timers won't produce any video frames.
Attachment #8598378 - Flags: feedback?(pehrsons)
Flags: needinfo?(jgilbert)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #85)
> Comment on attachment 8598378 [details] [diff] [review]
> 0001-Add-WebGLContext-GetInternalImage.patch
> 
> Review of attachment 8598378 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This requires `{preserveDrawingBuffer: true}` to work. How can we make it
> work without it?
> 
> Run `./mach mochitest-plain --subsuite webgl
> dom/canvas/test/webgl-mochitest/test_capture.html` and it'll fail because of
> this. The streams driven by timers won't produce any video frames.

Do we need to do this based on timers? If matching the framerate is ok (or preferable), we should probably do that. Asking for the 'current frame' at 60Hz is probably the wrong way to do this, it seems to me.
Flags: needinfo?(jgilbert)
(In reply to Jeff Gilbert [:jgilbert] from comment #86)
> Do we need to do this based on timers? If matching the framerate is ok (or
> preferable), we should probably do that. Asking for the 'current frame' at
> 60Hz is probably the wrong way to do this, it seems to me.

IIRC the proposed API includes timer-based frame sampling, as well a JS-driven sampling (i.e. draw a new frame a video, then strobe the canvas to generate a frame of data to the MediaStream).  For timer-based, generally the presumption is that the sampling rate would <= the generated rate (and it probably makes sense to not generate an output frame if the canvas hasn't changed).  This is still at the open proposal/pull-request stage, so input into what's useful and what makes sense (from a standards POV) would help.

For example, it might be ok (for timer-based) to basically set a flag to generate a frame at the next stable canvas state after the timer fires (I don't know enough about WebGL to understand the details of what would work for an interaction model).  This would cleanly handle the "requesting frame rates above the rate it's generated", for example.
Jeff, if you want to chat about what we might do to make this more performant, or better in any way, let's do that.

I've always considered the timer to be informative only.  If, for instance, we only used the timer to determine whether the next update might be captured, I think we'd be OK.  (The consequence being that actual frame rate <= requested frame rate.)  I hold the pen on the spec, so we have some ability to fix it.
OK, I just had a chat with Jeff and the conclusion was something like this:

1. Land the code with the caveats (i.e., note that preserveDrawingBuffer needs to be true for this to work).  We should fail the call to captureStream if preserveDrawingBuffer is not true.

2. File a follow-up to deal with preserveDrawingBuffer: false.  This will need additional work.  Jeff suggested that using TextureClient might be the right way to capture this information; but that it might not be trivial to convert the results into an Image so that the MSG can handle the data.

3. Update the spec to ensure that an exact timer is not necessary.  We should be able to set a flag on the timer that causes the next rendered frame to be captured, rather than pulling immediately when the timer pops.  It might be possible to pull from the front buffer, but we don't necessarily need to commit to doing so.

I can do the third item.
Attachment #8590988 - Flags: review?(martin.thomson)
Attachment #8590988 - Flags: review?(jgilbert)
Attachment #8590988 - Flags: review+
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

/r/6907 - Bug 1032848 - Part 1. Implement WebIDL for HTMLCanvasElement::CaptureStream. r=smaug,mt
/r/6909 - Bug 1032848 - Part 2. Implement HTMLCanvasElement::CaptureStream. r=mt,jesup,jgilbert,gwright
/r/6911 - Bug 1032848 - Part 3. Add tests for HTMLCanvasElement::CaptureStream. r=mt,jgilbert,jesup

Pull down these commits:

hg pull -r 37dc394fffb82492ee14f9f84dd3ff9b07dae264 https://reviewboard-hg.mozilla.org/gecko/
(In reply to Martin Thomson [:mt] from comment #89)
> OK, I just had a chat with Jeff and the conclusion was something like this:
> 
> 1. Land the code with the caveats (i.e., note that preserveDrawingBuffer
> needs to be true for this to work).  We should fail the call to
> captureStream if preserveDrawingBuffer is not true.

Here's the first item done. Martin and Jeff, please review my last changes to part 2 and 3 (between revisions 4 and 5 in reviewboard). I incorporated Jeff's patch by removing HTMLCanvasElement::GetInternalImage() and lifting out its logic to CanvasCaptureMediaStream.
(In reply to Martin Thomson [:mt] from comment #89)
> Jeff suggested that using TextureClient might be the
> right way to capture this information; but that it might not be trivial to
> convert the results into an Image so that the MSG can handle the data.

It should be easy, unless the TextureClient implementation does not own its gpu texture (which Jeff is addressing for TextureClientSharedSurface in bug 1144906).
Attachment #8590988 - Flags: review?(martin.thomson) → review+
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

https://reviewboard.mozilla.org/r/6905/#review6639

Ship It!
I have a try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=483aa1f2470d

I'm currently trying to resolve the two remaining issues.

On Windows, I believe a mix of DataSourceSurface::GetData() and DataSourceSurface::Map() is causing unfortunate issues. On Windows (well, at least) it seems like you can never Map() after you've done a GetData().

On B2G, for some reason the HTMLImageElement never raises the "load" event. Trying to see now if we're getting "error" instead or what's up there.
Blocks: 1161913
Blocks: 1162353
Depends on: 1162357
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

/r/6907 - Bug 1032848 - Part 1. Implement WebIDL for HTMLCanvasElement::CaptureStream. r=smaug,mt
/r/6909 - Bug 1032848 - Part 2. Implement HTMLCanvasElement::CaptureStream. r=mt,jesup,jgilbert,gwright
/r/6911 - Bug 1032848 - Part 3. Add tests for HTMLCanvasElement::CaptureStream. r=mt,jgilbert,jesup

Pull down these commits:

hg pull -r 0bd53a3228fa33040674753ee030ffe08944e81e https://reviewboard-hg.mozilla.org/gecko/
Attachment #8590988 - Flags: review+
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

Conclusion from bug 1162357: If we mimick the same behaviour as Canvas2D's drawImage() - i.e., call DrawTarget::OptimizeSourceSurface() with the surface from the canvas - we won't end up competing for DataSourceSurface::Map(). That solves the last issues on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b27063c13ec

The WIP patch from try is incorporated on reviewboard. jgilbert, could you review that along with my previous request? All in all, it should be the interdiff between revision 4 and 6.

This patch carries over r=smaug,mt for part 1; r=mt,jesup,gwright for part 2; r=mt,jesup for part 3.
Attachment #8590988 - Flags: review+
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

https://reviewboard.mozilla.org/r/6905/#review7305

It seems promise-heavy, but it's that sort of code, I guess.

::: dom/canvas/test/webgl-mochitest/test_capture.html:171
(Diff revision 6)
> +  var indexArr = new Uint16Array([
> +    0, 1, 2,
> +    3,
> +  ]);
> +  var indexBuff = gl.createBuffer();
> +  gl.bindBuffer(gl.ELEMENT_ARRAY_BUFFER, indexBuff);
> +  gl.bufferData(gl.ELEMENT_ARRAY_BUFFER, indexArr, gl.STATIC_DRAW);

Don't use an index buffer if you don't need it.
If you remove these lines, the program still works.

::: dom/canvas/test/captureStream_common.js:150
(Diff revision 6)
> +CaptureStreamTestHelperWebGL.prototype = Object.create(CaptureStreamTestHelper.prototype);
> +CaptureStreamTestHelperWebGL.prototype.constructor = CaptureStreamTestHelperWebGL;
> +
> +/* Set the (uniform) color location for future draw calls. */
> +CaptureStreamTestHelperWebGL.prototype.setFragmentColorLocation = function(colorLocation) {
> +  this.colorLocation = colorLocation;
> +};
> +
> +/* Clear the given WebGL context with |color|. */
> +CaptureStreamTestHelperWebGL.prototype.clearColor = function(canvas, color) {
> +  info("WebGL: clearColor(" + color.name + ")");
> +  var gl = canvas.getContext('webgl');
> +  var conv = color.data.map(i => i / 255.0);
> +  gl.clearColor(conv[0], conv[1], conv[2], conv[3]);
> +  gl.clear(gl.COLOR_BUFFER_BIT);
> +};
> +
> +/* Set an already setFragmentColorLocation() to |color| and drawArrays() */
> +CaptureStreamTestHelperWebGL.prototype.drawColor = function(canvas, color) {
> +  info("WebGL: drawArrays(" + color.name + ")");
> +  var gl = canvas.getContext('webgl');
> +  var conv = color.data.map(i => i / 255.0);
> +  gl.uniform4f(this.colorLocation, conv[0], conv[1], conv[2], conv[3]);
> +  gl.drawArrays(gl.TRIANGLE_STRIP, 0, 4);

Why is this in a common include instead of in the file it's used in? It's strictly more confusing to have this disjoint from its single caller. There's a lot of magic interdependencies here, which code locality makes easier to see.
Attachment #8590988 - Flags: review?(jgilbert)
https://reviewboard.mozilla.org/r/6905/#review7309

> Why is this in a common include instead of in the file it's used in? It's strictly more confusing to have this disjoint from its single caller. There's a lot of magic interdependencies here, which code locality makes easier to see.

Because it's not a single caller. Both dom/canvas/test/webgl-mochitest/test_capture.html and dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_webgl.html call this.

> Don't use an index buffer if you don't need it.
> If you remove these lines, the program still works.

Nice catch! This is leftovers from an earlier revision. Verified locally.
Attachment #8590988 - Flags: review+ → review?(jgilbert)
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

/r/6907 - Bug 1032848 - Part 1. Implement WebIDL for HTMLCanvasElement::CaptureStream. r=smaug,mt
/r/6909 - Bug 1032848 - Part 2. Implement HTMLCanvasElement::CaptureStream. r=mt,jesup,jgilbert,gwright
/r/6911 - Bug 1032848 - Part 3. Add tests for HTMLCanvasElement::CaptureStream. r=mt,jgilbert,jesup

Pull down these commits:

hg pull -r 10feb229fd88943a7811f88da5213865f3eae72b https://reviewboard-hg.mozilla.org/gecko/
Attachment #8590988 - Flags: review?(jgilbert) → review+
Comment on attachment 8590988 [details]
MozReview Request: bz://1032848/pehrsons

https://reviewboard.mozilla.org/r/6905/#review7385

Ship It!
Attachment #8598378 - Attachment is obsolete: true
Keywords: checkin-needed
No longer depends on: 1162357
See Also: → 1162357
backlog: --- → webRTC+
Flags: firefox-backlog+
(In reply to Carsten Book [:Tomcat] from comment #102)
> https://hg.mozilla.org/mozilla-central/rev/fc943ddde1c4

This changeset was missing an "override" annotation after the Notify method-declaration,  which causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+).

I pushed a followup to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/553e1492ae9c
(In reply to Daniel Holbert [:dholbert] from comment #103)
> (In reply to Carsten Book [:Tomcat] from comment #102)
> > https://hg.mozilla.org/mozilla-central/rev/fc943ddde1c4
> 
> This changeset was missing an "override" annotation after the Notify
> method-declaration,  which causes clang 3.6 & newer to spam a
> "-Winconsistent-missing-override" build warning, which busts
> --enable-warnings-as-errors builds (with clang 3.6+).
> 
> I pushed a followup to add that keyword, with blanket r+ that ehsan granted
> me for fixes of this sort over in bug 1126447 comment 2:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/553e1492ae9c

Sorry for that, and thanks for fixing!
Depends on: 1169125
Depends on: 1169126

        Attachment #8590988 -
        Attachment is obsolete: true

        Attachment #8618183 -
        Flags: review+

        Attachment #8618184 -
        Flags: review+

        Attachment #8618185 -
        Flags: review+

    
  
Depends on: 1173686

    
  
Depends on: 1174056

    
    

    
  
Works like a charm. http://output.jsbin.com/cuvocu/

Thanks, everybody!

    
    

    
  
(In reply to brian from comment #111)
> Works like a charm. http://output.jsbin.com/cuvocu/
> 
> Thanks, everybody!

Very cool!

Note: this (and the base video chat example it's based on for peerjs) breaks if the person hitting "Call" is on 40 or 41, apparently due to the changes we've made to be more spec-compliant (firing onnegotiationneeded)

Likely some code to handle either variation is needed, as can be seen in http://jsfiddle.net/jib1/b6gLnbob/

This almost certainly needs to go into peerjs itself.

    
    

    
  
Brian - can you use an updated version of peerjs that works with Firefox 41?  (See comment 112)

Again, thanks for a cool demo!
Flags: needinfo?(brian)

    
    

    
  
Brian - perhaps SkyWay (fork of peerjs that appears actively maintained)

    
    

    
  
Hey Randell,

I updated the code to use SkyWay, but I still can't make outgoing connections from Nightly. Do you know of another easy library that also comes with a server to negotiate the connection?
Flags: needinfo?(brian)

    
    

    
  
Thanks!  the bug in peerjs is likely still in skyway - but since that's maintained, it should be possible to get a fix into that.  Almost certainly it's a problem in handling onnegotiationneeded vs createOffer.  bwc, drno, and jib may be able to help, or provide alternatives.

We showed your demo at our internal all-hands meeting this week in the Science Fair, and got a lot of positive response.
Flags: needinfo?(jib)
Flags: needinfo?(drno)
Flags: needinfo?(docfaraday)

    
    

    
  
See Bug 1175609 that I linked to for an example of the negotiationneeded problem and discussions about workarounds.
Flags: needinfo?(jib)

    
    

    
  
Fixing bug 1175609 should fix that problem, yes. It might be an easy fix to SkyWay, but you may run into other problems.
Flags: needinfo?(docfaraday)

    
    

    
  
Create an issue for Skyway's peerjs fork: https://github.com/nttcom/peerjs/issues/40 to make them aware of the problem.
Flags: needinfo?(drno)

    
    

    
  
FYI: turns out Skyway's peerjs 0.3 dist seems to have fixed the issue already.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: