Closed
Bug 1043808
Opened 10 years ago
Closed 10 years ago
Orange cleanup for Linux window/screensharing tests
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: jesup, Assigned: jesup)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 1 obsolete file)
(deleted),
patch
|
ted
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jhlin
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
pkerr
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
pkerr
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
pkerr
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8462354 -
Flags: review?(jolin)
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/94fea719a746 leave-open for the others
Whiteboard: [sceensharing-uplift] → [sceensharing-uplift][leave-open]
Updated•10 years ago
|
Attachment #8462353 -
Flags: review?(ted) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5618e86329 leave-open for last issue
https://hg.mozilla.org/mozilla-central/rev/94fea719a746 https://hg.mozilla.org/mozilla-central/rev/0c5618e86329
Updated•10 years ago
|
Priority: -- → P1
Target Milestone: --- → mozilla34
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8462355 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Whiteboard: [sceensharing-uplift][leave-open] → [sceensharing-uplift]
Updated•10 years ago
|
Attachment #8464223 -
Flags: review?(pkerr) → review+
Assignee | ||
Comment 11•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8464808 -
Flags: review?(pkerr)
Updated•10 years ago
|
Attachment #8464808 -
Flags: review?(pkerr) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8464940 -
Flags: review?(pkerr)
Updated•10 years ago
|
Attachment #8464940 -
Flags: review?(pkerr) → review+
Assignee | ||
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e65279c96603 https://hg.mozilla.org/integration/mozilla-inbound/rev/cfaf1dbaca7a
status-firefox33:
--- → affected
status-firefox34:
--- → affected
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eed9b58c0ce3 Bustage fix. Extra includes snuck in from testing
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e65279c96603 https://hg.mozilla.org/mozilla-central/rev/cfaf1dbaca7a https://hg.mozilla.org/mozilla-central/rev/eed9b58c0ce3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 16•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Attachment #8462354 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Attachment #8464223 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Attachment #8464808 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Attachment #8464940 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Whiteboard: [sceensharing-uplift] → [screensharing-uplift]
Assignee | ||
Comment 17•10 years ago
|
||
This bug was missed in previous requests because "screensharing-uplift" was mis-spelled
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8464940 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8464808 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8464223 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8462354 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8462353 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 18•10 years ago
|
||
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
Whiteboard: [screensharing-uplift]
QA Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•