1043808 - Orange cleanup for Linux window/screensharing tests

Status: RESOLVED FIXED

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

Description

[Randell Jesup [:jesup] (needinfo me)]

Fixing the window/screenshare tests (missing pref) in bug 1039666 exposed some Linux issues on Try. 

One was an nasty problem due to two files being compiled with different options, leading to the same structure having two different sizes.  Easily resolved by moving the compilation of the file to the right gypi file.

MediaPipeline was shown to have some issues with odd sizes for video frames.

Content analysis appears to have a missing null check, though more investigation as to why it was needed should be done.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Compile desktop_capture_impl.cc with the rest of desktop_capture

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Clean up rounding of sizes in MediaPipeline to handle odd sizes correctly

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: add null-checks to webrtc content analysis

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8462353 [details] [diff] [review]
Compile desktop_capture_impl.cc with the rest of desktop_capture

Compiling desktop_capture_impl with video meant it didn't see USE_X11, etc, and when it included objects with #ifdef USE_X11 .... boom.

Comment 5

[John Lin [:jhlin][:jolin]]

Comment on attachment 8462354 [details] [diff] [review]
Clean up rounding of sizes in MediaPipeline to handle odd sizes correctly

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

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +1111,5 @@
>    if (!enabled_ || chunk.mFrame.GetForceBlack()) {
> +    gfx::IntSize size = img->GetSize();
> +    uint32_t yPlaneLen = YSIZE(size.width, size.height);
> +    uint32_t cbcrPlaneLen = 2 * CRSIZE(size.width, size.height);
> +    uint32_t length = I420SIZE(size.width, size.height);

|yPlaneLen + cbcrPlaneLen| to save some calculation.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94fea719a746
leave-open for the others

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5618e86329
leave-open for last issue

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/94fea719a746
https://hg.mozilla.org/mozilla-central/rev/0c5618e86329

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add null-checks to content_analysis; move ownership from raw to scoped_ptr's

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8464223 [details] [diff] [review]
Add null-checks to content_analysis; move ownership from raw to scoped_ptr's

https://tbpl.mozilla.org/?tree=Try&rev=86dcc5a1ad9a
This has a patch that runs the window/screen-sharing tests 10 times each for each mochitest-3 run.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Don't allow content_analysis to half-initialize on small capture sizes

The fundamental error was that on small share sizes, it would half-initialize but leave the structures/buffers unallocated.  There doesn't appear to be off-capture-thread access except to create it and destroy it (after shutdown), so critical sections shouldn't be needed.  Green in tries that ran the tests hundreds of times.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: remove assertions that video is an even number of pixels

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e65279c96603
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfaf1dbaca7a

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eed9b58c0ce3
Bustage fix.  Extra includes snuck in from testing

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e65279c96603
https://hg.mozilla.org/mozilla-central/rev/cfaf1dbaca7a
https://hg.mozilla.org/mozilla-central/rev/eed9b58c0ce3

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8462353 [details] [diff] [review]
Compile desktop_capture_impl.cc with the rest of desktop_capture

For all the small patches on this bug

Approval Request Comment
[Feature/regressing bug #]: screensharing

[User impact if declined]: we'd need to pref off screensharing on 33 due to ASAN issues, especially for this patch.

[Describe test coverage new/current, TBPL]: covered by screensharing tests

[Risks and why]: low risk to very low - baked on m-c.  Ran Try tests with the screen/window sharing tests "looped" to run 10 iterations of each per run (and removed other tests), and retriggered ~10+ times for each platform for landing on m-c.  Several of the patches are trivial.

[String/UUID change made/needed]: none

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

This bug was missed in previous requests because "screensharing-uplift" was mis-spelled

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Temp disable of tests a=kwierso:
https://hg.mozilla.org/releases/mozilla-aurora/rev/dbfe26a84fcf

Uplift and re-enable tests:
https://hg.mozilla.org/releases/mozilla-aurora/rev/2cefba6dc412
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ca5b28fb162
https://hg.mozilla.org/releases/mozilla-aurora/rev/b13ba3879b2e
https://hg.mozilla.org/releases/mozilla-aurora/rev/d08b3b54c0f5
https://hg.mozilla.org/releases/mozilla-aurora/rev/62a14ece9ea8