Closed Bug 1043808 Opened 5 years ago Closed 5 years ago

Orange cleanup for Linux window/screensharing tests

Categories

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

All
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

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 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.
Attachment #8462353 - Flags: review?(ted)
Attachment #8462354 - Flags: review?(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.
Attachment #8462354 - Flags: review?(jolin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/94fea719a746
leave-open for the others
Whiteboard: [sceensharing-uplift] → [sceensharing-uplift][leave-open]
Attachment #8462353 - Flags: review?(ted) → review+
Priority: -- → P1
Target Milestone: --- → mozilla34
Attachment #8462355 - Attachment is obsolete: true
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.
Attachment #8464223 - Flags: review?(pkerr)
Whiteboard: [sceensharing-uplift][leave-open] → [sceensharing-uplift]
Attachment #8464223 - Flags: review?(pkerr) → review+
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.
Attachment #8464808 - Flags: review?(pkerr)
Attachment #8464808 - Flags: review?(pkerr) → review+
Attachment #8464940 - Flags: review?(pkerr)
Attachment #8464940 - Flags: review?(pkerr) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/eed9b58c0ce3
Bustage fix.  Extra includes snuck in from testing
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
Attachment #8462353 - Flags: approval-mozilla-aurora?
Attachment #8462354 - Flags: approval-mozilla-aurora?
Attachment #8464223 - Flags: approval-mozilla-aurora?
Attachment #8464808 - Flags: approval-mozilla-aurora?
Attachment #8464940 - Flags: approval-mozilla-aurora?
Whiteboard: [sceensharing-uplift] → [screensharing-uplift]
This bug was missed in previous requests because "screensharing-uplift" was mis-spelled
Attachment #8464940 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8464808 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8464223 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8462354 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8462353 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.