1379743 - Screensharing with resize constraints is broken (graphics garbage) (regression)

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

STRs (tested on OSX and Windows 10):

 1. Open https://jsfiddle.net/jib1/3xytpne5/ and share screen

Expected result:
  320x200 video of desktop (scaled down).

Actual result:
  320x200 pink video garbage

Workaround:
 - Remove the width and height constraints, or have them match the desktop's natural size.
   Then the desktop video shows just fine.

Regression range:

15:11.95 INFO: Last good revision: c7428449127566105bdd94b2823b01e0e5e007d5
15:11.95 INFO: First bad revision: 5bbdb7d36ee3c136a0ed03be9d5b012d05dfd08e
15:11.95 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c7428449127566105bdd94b2823b01e0e5e007d5&tochange=5bbdb7d36ee3c136a0ed03be9d5b012d05dfd08e

Looks like a regression from bug 1341285.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

We should also contemplate a test that will catch this in the future.

Comment 2

[Dan Minor [:dminor]]

I can have a look, I worked on this part of the 57 update.

Comment 3

[Dan Minor [:dminor]]

Attachment: Recalculate stride when scaling capture image

I guess the natural way to test this in automation would be to add a reftest. I don't think we currently have any for WebRTC and I'm not sure how reliable it would be.

Since we use https://mozilla.github.io/webrtc-landing/ to test when landing a new webrtc.org update we could add an option there to force scaling when doing desktop capture.

Please let me know what you think.

Comment 4

[Dan Minor [:dminor]]

Attachment: Recalculate stride when scaling capture image

Accidentally ran hg out rather than hg export when generating first version of the patch.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Our current screenshare resize test is quite cursory, merely testing the "resize" event fires. I've been meaning to improve this to at least verify that metrics like dimensions are within a valid envelope.

I also know Andreas has written some successful mochitests to verify pixels using waitForPixelColor() [2], isPixel() and the like, though there's a real challenge with real screenshots compared to fake cameras or canvas-generated streams. One idea I had involved drawing a big red splotch on a page and somehow locating it in both the scaled and unscaled version, or similar means for comparing two images (including maybe post-scaling the unscaled version in canvas). Other ideas welcome. Happy to open a separate bug for this, since it may be a bit involved.

[1] https://dxr.mozilla.org/mozilla-central/rev/b07db5d650b7056c78ba0dbc409d060ec4e922cd/dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html#76
[2] https://dxr.mozilla.org/mozilla-central/rev/b07db5d650b7056c78ba0dbc409d060ec4e922cd/dom/canvas/test/captureStream_common.js#146

Comment 6

[Dan Minor [:dminor]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> Our current screenshare resize test is quite cursory, merely testing the
> "resize" event fires. I've been meaning to improve this to at least verify
> that metrics like dimensions are within a valid envelope.
> 
> I also know Andreas has written some successful mochitests to verify pixels
> using waitForPixelColor() [2], isPixel() and the like, though there's a real
> challenge with real screenshots compared to fake cameras or canvas-generated
> streams. One idea I had involved drawing a big red splotch on a page and
> somehow locating it in both the scaled and unscaled version, or similar
> means for comparing two images (including maybe post-scaling the unscaled
> version in canvas). Other ideas welcome. Happy to open a separate bug for
> this, since it may be a bit involved.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> b07db5d650b7056c78ba0dbc409d060ec4e922cd/dom/media/tests/mochitest/
> test_getUserMedia_basicScreenshare.html#76
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> b07db5d650b7056c78ba0dbc409d060ec4e922cd/dom/canvas/test/
> captureStream_common.js#146

I filed Bug 1380346 to track improving the testing for this.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ace13e49c0fb
Recalculate stride when scaling desktop capture; r=jesup

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ace13e49c0fb