bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Handle preserveDrawingBuffer: false when capturing to a stream

RESOLVED FIXED in Firefox 43

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: pehrsons, Assigned: pehrsons)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox43 fixed)

Details

(Whiteboard: [gfx-noted][games:p1][platform-rel-Games])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
In its first version, HTMLCanvasElement::CaptureStream for WebGLContexts only work when `preserveDrawingBuffer` is true. This is a follow-up to handle the preserveDrawingBuffer: false case.

From bug 1032848 comment 89 (mt):
> 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.

Also worth noting bug 1032848 comment 92 (nical):
> It should be easy, unless the TextureClient implementation does not own its gpu texture (which Jeff is addressing for TextureClientSharedSurface in bug 1144906).
Hi Milan -- We need this bug fixed so we can complete the canvas capture feature (that Andreas Pehrson implemented).  The feature works for preserveDrawingBuffer:true, but there are cases where it can't capture. Can you find an assignee for this?
Flags: needinfo?(milan)
What train?
Flags: needinfo?(milan)
Also, where is this feature in the priority order?
Flags: needinfo?(mreavy)
It landed in Fx41 (current Nightly), but it's behind a pref due to a blocking bug that pehrsons is fixing.  Having a working implementation of canvas.captureStream is pretty important to drive the spec forward (i.e. to get other vendors to implement it and show leadership on the spec).  We could probably un-pref without this fix, but we'd prefer not to.
Flags: needinfo?(mreavy)
Flags: needinfo?(smooney)
(Assignee)

Updated

3 years ago
Blocks: 1177276

Updated

3 years ago
Whiteboard: [gfx-noted]
(Assignee)

Comment 5

3 years ago
I have started looking at this and have some progress. Patches will come pretty soon.
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
(Assignee)

Comment 6

3 years ago
Created attachment 8644968 [details]
MozReview Request: Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?jgilbert

Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?
(Assignee)

Comment 7

3 years ago
Created attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?
(Assignee)

Comment 8

3 years ago
Created attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?
We used to fully guarantee the order of requestFrame() and draw calls.
For instance:
```
ctx.draw(red);
stream.requestFrame();
ctx.draw(green);
```
would guarantee that a red frame ended up in the stream, and not the
green unless another frame was requested.

Now with frames being requested and pushed out on next refresh, we can
only guarantee that everything up to the requestFrame() call is included
in the next frame. Everything after the requestFrame() and before the
next refresh (stable state in most cases) will now also be inevitably
included.
(Assignee)

Comment 9

3 years ago
Created attachment 8644971 [details]
MozReview Request: Bug 1161913 - Part 4. Garbage collection may happen at any time. Don't assert in CanvasCaptureMediaStream. r?

Bug 1161913 - Part 4. Garbage collection may happen at any time. Don't assert in CanvasCaptureMediaStream. r?
(Assignee)

Comment 10

3 years ago
These patches should fix the `preserveDrawingBuffer: true` case. At least on my Mac, running on other platforms here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dc3e79e780b

It constrains the stream a bit more than before (see comment 8) by that a requested frame will only capture the canvas' *next drawn* frame. With a slow frame rate for instance, it's possible to miss a frame, and if no drawing is done for a while, the stream and the canvas will appear out of sync. If clear to the developer though it wouldn't be a problem (auto or manual mode fixes it) - perhaps make it clearer in the spec?

Martin & Jeff, double checking with you that this is according to the spec and your discussions. If good, I'll fix up documenting the code and request reviews next.
Flags: needinfo?(smooney)
Flags: needinfo?(martin.thomson)
Flags: needinfo?(jgilbert)
Yep, that sounds exactly.  Andreas, I've made a few (proposed) changes to the spec.  Can you review these please?
Flags: needinfo?(martin.thomson) → needinfo?(pehrsons)
(Assignee)

Comment 12

3 years ago
(In reply to Martin Thomson [:mt:] from comment #11)
> Yep, that sounds exactly.  Andreas, I've made a few (proposed) changes to
> the spec.  Can you review these please?

Great! I added some comments. It would be good if Jeff looked it through as well.
Flags: needinfo?(pehrsons)
(Assignee)

Updated

3 years ago
Attachment #8644968 - Attachment description: MozReview Request: Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r? → MozReview Request: Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?jgilbert
Attachment #8644968 - Flags: review?(jgilbert)
(Assignee)

Comment 13

3 years ago
Comment on attachment 8644968 [details]
MozReview Request: Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?jgilbert

Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?jgilbert
(Assignee)

Comment 14

3 years ago
Comment on attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert
Attachment #8644969 - Attachment description: MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r? → MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert
Attachment #8644969 - Flags: review?(martin.thomson)
Attachment #8644969 - Flags: review?(jgilbert)
(Assignee)

Comment 15

3 years ago
Comment on attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt
We used to fully guarantee the order of requestFrame() and draw calls.
For instance:
```
ctx.draw(red);
stream.requestFrame();
ctx.draw(green);
```
would guarantee that a red frame ended up in the stream, and not the
green unless another frame was requested.

Now with frames being requested and pushed out on next refresh, we can
only guarantee that everything up to the requestFrame() call is included
in the next frame. Everything after the requestFrame() and before the
next refresh (stable state in most cases) will now also be inevitably
included.
Attachment #8644970 - Attachment description: MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r? → MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt
Attachment #8644970 - Flags: review?(martin.thomson)
(Assignee)

Updated

3 years ago
Attachment #8644971 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
These changes are better aligned with Martin's latest proposal on next-frame-capturing: https://github.com/w3c/mediacapture-fromelement/pull/11.
(Assignee)

Updated

3 years ago
Blocks: 1152298

Updated

3 years ago
Attachment #8644970 - Flags: review?(martin.thomson) → review+
Comment on attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

https://reviewboard.mozilla.org/r/15341/#review15555

::: dom/canvas/test/test_capture.html:40
(Diff revision 2)
> +  var drawing = h.startDrawing(h.drawColor.bind(h, c, h.green));

Just use an arrow like below:
var drawing = h.startDrawing(_ => h.drawColor(c, h.green));

::: dom/canvas/test/test_capture.html:48
(Diff revision 2)
> +    .then(() => drawing.stop());

Add a .catch() clause before the final .then to avoid getting into a sticky state if something fails here.

::: dom/canvas/test/test_capture.html:62
(Diff revision 2)
>  function checkDrawImageNotCleanRed() {

Same comments as above here.

::: dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_2d.html:45
(Diff revision 2)
> +      stream.requestFrame();
>        h.drawColor(canvas, h.red);

This order *seems* wrong, even though it is not.  Did you do it in this order so that you can demonstrate that point?  If so, add a comment to that effect or someone will "fix" this.

::: dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_webgl.html:34
(Diff revision 2)
> +  canvas.style = "display:none";

canvas.style.display = 'none'

::: dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_webgl.html:94
(Diff revision 2)
>      function REQUEST_FRAME(test) {
>        test.pcLocal.canvasStream.requestFrame();
>      },
> +    function DRAW_LOCAL_RED() {
> +      h.drawColor(canvas, h.red);
> +    },

Same comment as above, though more important here.
Comment on attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

https://reviewboard.mozilla.org/r/15339/#review15559

This looks fine.  I was wondering if you had a test for when the canvas becomes write-only during capture.  It looks like what happens there is that you continue to see the last readable frame from the canvas.  I wonder if it is worth putting a black frame into the stream at that point.  If you think that's a good idea, or even possible, a follow-up bug is fine.

::: dom/html/HTMLCanvasElement.h:64
(Diff revision 2)
> +  /*
> +   * Indicates to the canvas whether or not this listener has requested a frame.
> +   */
> +  bool mFrameCaptureRequested;

Good hygiene suggests that you should have accessors for this.  Maybe
```
void RequestFrame() { mFrameRequested = true; }
bool FrameRequested() const { return mFrameRequested; }
```

::: dom/html/HTMLCanvasElement.cpp:1225
(Diff revision 2)
> +    if (listener->mFrameCaptureRequested) {

You need to test for the weak pointer being null too, or boom.

::: dom/media/CanvasCaptureMediaStream.h:81
(Diff revision 2)
> -  void SetImage(layers::Image* aImage);
> +  void SetImage(already_AddRefed<layers::Image> aImage);

If it were me, I would just use `const nsRefPtr<layers::Image>&`

::: dom/media/CanvasCaptureMediaStream.cpp:41
(Diff revision 2)
> +  void SetImage(already_AddRefed<Image> aImage)
> +  {
>      MutexAutoLock lock(mMutex);
> -    mDriver = nullptr;
> +    mImage = aImage;
>    }

Do you want to store the capture time at this point?  Or do we let the captured frame enter the graph at whatever time it actually arrives?
Attachment #8644969 - Flags: review?(martin.thomson) → review+
(Assignee)

Comment 19

3 years ago
https://reviewboard.mozilla.org/r/15339/#review15559

I'ts very possible to do, but I haven't seen any convincing arguments for why it should become black. More obvious to the user?

The spec says "The captured stream immediately ceases to capture content from the canvas if the origin-clean flag of the canvas becomes false at any time." which we are still following.

> You need to test for the weak pointer being null too, or boom.

Ugh, yes!

> Do you want to store the capture time at this point?  Or do we let the captured frame enter the graph at whatever time it actually arrives?

It will be picked up by the graph on its next iteration (happens every 10ms, typically). Frame timestamps in the output stream are handled automagically by the graph.
(Assignee)

Comment 20

3 years ago
Comment on attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert
Attachment #8644969 - Flags: review+ → review?(martin.thomson)
(Assignee)

Comment 21

3 years ago
Comment on attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt
We used to fully guarantee the order of requestFrame() and draw calls.
For instance:
```
ctx.draw(red);
stream.requestFrame();
ctx.draw(green);
```
would guarantee that a red frame ended up in the stream, and not the
green unless another frame was requested.

Now with frames being requested and pushed out on next refresh, we can
only guarantee that everything up to the requestFrame() call is included
in the next frame. Everything after the requestFrame() and before the
next refresh (stable state in most cases) will now also be inevitably
included.
Attachment #8644970 - Flags: review+ → review?(martin.thomson)
(Assignee)

Comment 22

3 years ago
Comment on attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

Carrying forward r=mt
Attachment #8644970 - Flags: review?(martin.thomson) → review+
(Assignee)

Comment 23

3 years ago
Comment on attachment 8644968 [details]
MozReview Request: Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?jgilbert

Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?jgilbert
(Assignee)

Comment 24

3 years ago
Comment on attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert
(Assignee)

Comment 25

3 years ago
Comment on attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r=mt
We used to fully guarantee the order of requestFrame() and draw calls.
For instance:
```
ctx.draw(red);
stream.requestFrame();
ctx.draw(green);
```
would guarantee that a red frame ended up in the stream, and not the
green unless another frame was requested.

Now with frames being requested and pushed out on next refresh, we can
only guarantee that everything up to the requestFrame() call is included
in the next frame. Everything after the requestFrame() and before the
next refresh (stable state in most cases) will now also be inevitably
included.
Attachment #8644970 - Attachment description: MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt → MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r=mt
Attachment #8644970 - Flags: review+ → review?(martin.thomson)
(Assignee)

Comment 26

3 years ago
Comment on attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r=mt
We used to fully guarantee the order of requestFrame() and draw calls.
For instance:
```
ctx.draw(red);
stream.requestFrame();
ctx.draw(green);
```
would guarantee that a red frame ended up in the stream, and not the
green unless another frame was requested.

Now with frames being requested and pushed out on next refresh, we can
only guarantee that everything up to the requestFrame() call is included
in the next frame. Everything after the requestFrame() and before the
next refresh (stable state in most cases) will now also be inevitably
included.
Attachment #8644970 - Flags: review?(martin.thomson)
(Assignee)

Comment 27

3 years ago
Comment on attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

Did a small update to fix a build bustage. I don't get along well with mozreview at the moment.. Sorry for the spam.
Attachment #8644970 - Flags: review+
You can remove reviewers from the commit prior to publishing to avoid spam.  I agree though, RB should recognize r= and that the review was previously given and omit the reviewer.  See bug 1192958
(Assignee)

Comment 29

3 years ago
Yep, but then I haven't used mozreview much lately and saw this new shiny "Do you want to publish now?" in the CLI. :-)

Updated

3 years ago
Attachment #8644969 - Flags: review?(martin.thomson) → review+
Comment on attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

https://reviewboard.mozilla.org/r/15339/#review15955
https://reviewboard.mozilla.org/r/15339/#review15559

That's fine.  At some level, whatever we choose here becomes the new normal.
(Assignee)

Comment 32

3 years ago
Try push looks quite good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=72dca6c3386d

Just those two intermittents we seem to be triggering...
Depends on: 1072313, 1196689
Comment on attachment 8644968 [details]
MozReview Request: Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?jgilbert

https://reviewboard.mozilla.org/r/15337/#review16773
Attachment #8644968 - Flags: review?(jgilbert) → review+
Comment on attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

https://reviewboard.mozilla.org/r/15339/#review16775

Partial review.

::: dom/html/HTMLCanvasElement.cpp:58
(Diff revision 4)
> +  explicit RequestedFrameRefreshObserver(HTMLCanvasElement* aElement,

'explicit' on a two-arg ctor?

::: dom/html/HTMLCanvasElement.cpp:169
(Diff revision 4)
> +  HTMLCanvasElement* mElement;

HTMLCanvasElement\* const mOwningElement

It's a bare pointer so it \*is\* the owner (/back-ref), right?

::: dom/html/HTMLCanvasElement.cpp:1248
(Diff revision 4)
> +  for (int i = mRequestedFrameListeners.Length() - 1; i >= 0; --i) {

If you're doing a non-standard for loop, comment saying why.

::: dom/html/HTMLCanvasElement.cpp:1256
(Diff revision 4)
> +    if (listener->FrameCaptureRequested()) {
> +      nsRefPtr<Image> imageRefCopy = image.get();
> +      listener->NewFrame(imageRefCopy.forget());

NewFrame() should check FrameCaptureRequested() itself.
Attachment #8644969 - Flags: review?(jgilbert)
(Assignee)

Comment 35

3 years ago
Comment on attachment 8644968 [details]
MozReview Request: Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?jgilbert

Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?jgilbert
Attachment #8644968 - Flags: review+
(Assignee)

Comment 36

3 years ago
Comment on attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert
Attachment #8644969 - Flags: review+ → review?(jgilbert)
(Assignee)

Comment 37

3 years ago
Comment on attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r=mt
We used to fully guarantee the order of requestFrame() and draw calls.
For instance:
```
ctx.draw(red);
stream.requestFrame();
ctx.draw(green);
```
would guarantee that a red frame ended up in the stream, and not the
green unless another frame was requested.

Now with frames being requested and pushed out on next refresh, we can
only guarantee that everything up to the requestFrame() call is included
in the next frame. Everything after the requestFrame() and before the
next refresh (stable state in most cases) will now also be inevitably
included.
Attachment #8644970 - Flags: review+
(Assignee)

Comment 38

3 years ago
https://reviewboard.mozilla.org/r/15339/#review16807

::: dom/media/CanvasCaptureMediaStream.cpp:149
(Diff revisions 4 - 5)
> +    if (!mFrameCaptureRequested) {

This doesn't work - we need to assign the `already_AddRefed` ptr before leaving the scope. I have it fixed and will update it after your next review Jeff.
Comment on attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

https://reviewboard.mozilla.org/r/15339/#review17355

The WebGL bits look good. Re-ask for review if you want me to wade into the dom/media bits.

::: dom/html/HTMLCanvasElement.cpp:97
(Diff revision 5)
> +    RefPtr<DataSourceSurface> copy;
> +    {
> +      DataSourceSurface::ScopedMap read(data, DataSourceSurface::READ);
> +      if (!read.IsMapped()) {
> +        return;
> +      }
> +
> +      copy = Factory::CreateDataSourceSurfaceWithStride(data->GetSize(),
> +                                                        data->GetFormat(),
> +                                                        read.GetStride());
> +      if (!copy) {
> +        return;
> +      }
> +
> +      DataSourceSurface::ScopedMap write(copy, DataSourceSurface::WRITE);
> +      if (!write.IsMapped()) {
> +        return;
> +      }
> +
> +      MOZ_ASSERT(read.GetStride() == write.GetStride());
> +      MOZ_ASSERT(data->GetSize() == copy->GetSize());
> +      MOZ_ASSERT(data->GetFormat() == copy->GetFormat());
> +
> +      memcpy(write.GetData(), read.GetData(),
> +             write.GetStride() * copy->GetSize().height);
> +    }

Pull this out into a function which makes the copy and returns a strong-ref to the copy.

::: dom/html/HTMLCanvasElement.cpp:104
(Diff revision 5)
> +      copy = Factory::CreateDataSourceSurfaceWithStride(data->GetSize(),
> +                                                        data->GetFormat(),
> +                                                        read.GetStride());

Why are you mixing references to `data` and `read`? Try to just `read`, since that's what we're copying from.

::: dom/html/HTMLCanvasElement.cpp:128
(Diff revision 5)
> +  void Forget()

It looks like this would be better named "DetachFromRefreshDriver()"?

::: dom/html/HTMLCanvasElement.cpp:150
(Diff revision 5)
> +  void Deregister()

Unregister instead of Deregister.

::: dom/html/HTMLCanvasElement.cpp:172
(Diff revision 5)
> +  nsRefPtr<nsRefreshDriver> mRefreshDriver;

Prefer RefPtr in new code.

::: dom/html/HTMLCanvasElement.cpp:1187
(Diff revision 5)
> +    MOZ_ASSERT(doc, "No owner doc");

MOZ_RELEASE_ASSERT, here and below.

::: dom/html/HTMLCanvasElement.cpp:1263
(Diff revision 5)
> +  if (mRequestedFrameListeners.Length() == 0) {

!Length(), instead of == 0
Attachment #8644969 - Flags: review?(jgilbert) → review+
A note - MOZ_CRASH gives us a bit more data than MOZ_RELEASE_ASSERT, so if these are the types of things where we'd actually want to crash - MOZ_CRASH may be a way to go.
(Assignee)

Comment 41

3 years ago
https://reviewboard.mozilla.org/r/15339/#review17355

Thanks. I'll take mt's r+ for dom/media.

> Pull this out into a function which makes the copy and returns a strong-ref to the copy.

Done.

> Why are you mixing references to `data` and `read`? Try to just `read`, since that's what we're copying from.

`read` is a (scoped) `MappedSurface` and contains only a pointer to `data`s underlying data storage, and its stride. I.e., the stuff not already provided by `data` that you need to read or write to it.

> It looks like this would be better named "DetachFromRefreshDriver()"?

Done.

> Unregister instead of Deregister.

Done.

> Prefer RefPtr in new code.

Done.

> MOZ_RELEASE_ASSERT, here and below.

Done. I didn't go with MOZ_CRASH as the asserts make for more concise and readable code.

> !Length(), instead of == 0

I changed this to `IsEmpty()`.
(Assignee)

Comment 42

3 years ago
Comment on attachment 8644968 [details]
MozReview Request: Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?jgilbert

Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?jgilbert
(Assignee)

Comment 43

3 years ago
Comment on attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert
Attachment #8644969 - Flags: review+
(Assignee)

Comment 44

3 years ago
Comment on attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt
We used to fully guarantee the order of requestFrame() and draw calls.
For instance:
```
ctx.draw(red);
stream.requestFrame();
ctx.draw(green);
```
would guarantee that a red frame ended up in the stream, and not the
green unless another frame was requested.

Now with frames being requested and pushed out on next refresh, we can
only guarantee that everything up to the requestFrame() call is included
in the next frame. Everything after the requestFrame() and before the
next refresh (stable state in most cases) will now also be inevitably
included.
Attachment #8644970 - Attachment description: MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r=mt → MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt
(Assignee)

Comment 45

3 years ago
Comment on attachment 8644968 [details]
MozReview Request: Bug 1161913 - Part 1. Add invalidation state for CaptureStream to Canvas and Contexts. r?jgilbert

Carries r=jgilbert. (c33)
Attachment #8644968 - Flags: review+
(Assignee)

Comment 46

3 years ago
Comment on attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

Carries r=mt,jgilbert (comment 18 and comment 39)
Attachment #8644969 - Flags: review+
(Assignee)

Comment 47

3 years ago
Comment on attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

Carries r=mt (comment 17)
Attachment #8644970 - Flags: review+
(Assignee)

Comment 48

3 years ago
Comment on attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert
Attachment #8644969 - Flags: review+
(Assignee)

Comment 49

3 years ago
Comment on attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt
We used to fully guarantee the order of requestFrame() and draw calls.
For instance:
```
ctx.draw(red);
stream.requestFrame();
ctx.draw(green);
```
would guarantee that a red frame ended up in the stream, and not the
green unless another frame was requested.

Now with frames being requested and pushed out on next refresh, we can
only guarantee that everything up to the requestFrame() call is included
in the next frame. Everything after the requestFrame() and before the
next refresh (stable state in most cases) will now also be inevitably
included.
Attachment #8644970 - Flags: review+
(Assignee)

Comment 50

3 years ago
Comment on attachment 8644969 [details]
MozReview Request: Bug 1161913 - Part 2. Request canvas to push out its next drawn frame instead of pulling it. r?mt,jgilbert

Try push [1] revealed a problem in static analysis.
Try push [2] with this commit shows that it's fixed.

Carries r=mt,jgilbert.


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=66f8b6d742c7
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=14c31870ae46
Attachment #8644969 - Flags: review+
(Assignee)

Comment 51

3 years ago
Comment on attachment 8644970 [details]
MozReview Request: Bug 1161913 - Part 3. Relax requestFrame ordering guarantee in tests. r?mt

Carries r=mt.
Attachment #8644970 - Flags: review+
(Assignee)

Comment 52

3 years ago
Please land this and bug 1152298 together.

See try pushes in comment 50. There are a bunch of tests there being close to permorange, but they're both outside of any code these patches touch, and seem to already have these high failure rates on m-i.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/261ea38e6cf6
https://hg.mozilla.org/mozilla-central/rev/444d8a31e893
https://hg.mozilla.org/mozilla-central/rev/c36e4b735006
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

3 years ago
Depends on: 1233613
Flags: needinfo?(jgilbert)
Whiteboard: [gfx-noted] → [gfx-noted][games:p1]
Whiteboard: [gfx-noted][games:p1] → [gfx-noted][games:p1][platform-rel-Games]
You need to log in before you can comment on or make changes to this bug.