1161913 - Handle preserveDrawingBuffer: false when capturing to a stream

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Andreas Pehrson [:pehrsons]]

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).

Comment 1

[Maire Reavy [:mreavy]]

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?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

What train?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Also, where is this feature in the priority order?

Comment 4

[Maire Reavy [: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.

Comment 5

[Andreas Pehrson [:pehrsons]]

I have started looking at this and have some progress. Patches will come pretty soon.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: 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?

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: 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?

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: 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.

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: 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?

Comment 10

[Andreas Pehrson [:pehrsons]]

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.

Comment 11

[Martin Thomson [:mt:]]

Yep, that sounds exactly.  Andreas, I've made a few (proposed) changes to the spec.  Can you review these please?

Comment 12

[Andreas Pehrson [:pehrsons]]

(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.

Comment 13

[Andreas Pehrson [:pehrsons]]

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

Comment 14

[Andreas Pehrson [:pehrsons]]

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

Comment 15

[Andreas Pehrson [:pehrsons]]

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.

Comment 16

[Andreas Pehrson [:pehrsons]]

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

Comment 17

[Martin Thomson [:mt:]]

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 18

[Martin Thomson [:mt:]]

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?

Comment 19

[Andreas Pehrson [:pehrsons]]

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.

Comment 20

[Andreas Pehrson [:pehrsons]]

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

Comment 21

[Andreas Pehrson [:pehrsons]]

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.

Comment 22

[Andreas Pehrson [:pehrsons]]

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

Carrying forward r=mt

Comment 23

[Andreas Pehrson [:pehrsons]]

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

Comment 24

[Andreas Pehrson [:pehrsons]]

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

Comment 25

[Andreas Pehrson [:pehrsons]]

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.

Comment 26

[Andreas Pehrson [:pehrsons]]

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.

Comment 27

[Andreas Pehrson [:pehrsons]]

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.

Comment 28

[Martin Thomson [:mt:]]

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

Comment 29

[Andreas Pehrson [:pehrsons]]

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

Comment 30

[Martin Thomson [:mt:]]

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

Comment 31

[Martin Thomson [:mt:]]

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

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

Comment 32

[Andreas Pehrson [:pehrsons]]

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

Just those two intermittents we seem to be triggering...

Comment 33

[Kelsey Gilbert [:jgilbert]]

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

Comment 34

[Kelsey Gilbert [:jgilbert]]

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.

Comment 35

[Andreas Pehrson [:pehrsons]]

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

Comment 36

[Andreas Pehrson [:pehrsons]]

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

Comment 37

[Andreas Pehrson [:pehrsons]]

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.

Comment 38

[Andreas Pehrson [:pehrsons]]

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 39

[Kelsey Gilbert [:jgilbert]]

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

Comment 40

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 41

[Andreas Pehrson [:pehrsons]]

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()`.

Comment 42

[Andreas Pehrson [:pehrsons]]

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

Comment 43

[Andreas Pehrson [:pehrsons]]

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

Comment 44

[Andreas Pehrson [:pehrsons]]

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.

Comment 45

[Andreas Pehrson [:pehrsons]]

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)

Comment 46

[Andreas Pehrson [:pehrsons]]

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)

Comment 47

[Andreas Pehrson [:pehrsons]]

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

Carries r=mt (comment 17)

Comment 48

[Andreas Pehrson [:pehrsons]]

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

Comment 49

[Andreas Pehrson [:pehrsons]]

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.

Comment 50

[Andreas Pehrson [:pehrsons]]

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

Comment 51

[Andreas Pehrson [:pehrsons]]

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

Carries r=mt.

Comment 52

[Andreas Pehrson [:pehrsons]]

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.

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/261ea38e6cf6
https://hg.mozilla.org/integration/mozilla-inbound/rev/444d8a31e893
https://hg.mozilla.org/integration/mozilla-inbound/rev/c36e4b735006

Comment 54

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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