1432793 - Crash in mozalloc_abort | abort | webrtc::ViEEncoder::ReconfigureEncoder

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P2)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-f052cd3b-5369-4b4b-975c-597490180124.
=============================================================

Spotted while looking at nightly Mac and Linux crash data: http://bit.ly/2DzYYHI. Not super high volume, but affects 58/59/60.

Top 10 frames of crashing thread:

0 libmozglue.dylib mozalloc_abort memory/mozalloc/mozalloc_abort.cpp:33
1 libmozglue.dylib abort memory/mozalloc/mozalloc_abort.cpp:80
2 XUL rtc::FatalMessage::~FatalMessage media/webrtc/trunk/webrtc/base/checks.cc:109
3 XUL rtc::FatalMessage::~FatalMessage media/webrtc/trunk/webrtc/base/checks.cc:102
4 XUL webrtc::ViEEncoder::ReconfigureEncoder media/webrtc/trunk/webrtc/video/vie_encoder.cc:424
5 XUL webrtc::ViEEncoder::EncodeVideoFrame media/webrtc/trunk/webrtc/video/vie_encoder.cc:553
6 XUL webrtc::ViEEncoder::EncodeTask::Run media/webrtc/trunk/webrtc/video/vie_encoder.cc:105
7 XUL rtc::TaskQueue::TaskContext::RunTask media/webrtc/trunk/webrtc/base/task_queue_gcd.cc:53
8  @0x7fff730cdd4f 
9  @0x7fff730e220b 

=============================================================

Comment 1

[Andreas Pehrson [:pehrsons]]

P2 for being low rate but unexpected until we know more.

Randell, weren't you looking at just this code recently?

Comment 2

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

I changed the RTC_DCHECK() into an RTC_CHECK() (release assert), which triggered this.  Likely these configs were failing before -- and the code would happily continue after failing to init the codec (which means no media would flow, and in some cases it might fail elsewhere.  Also, if it was vp8 failing to init, in some cases it might have been more problematic than this, so RTC_CHECK was a safety.

Knowing more about what caused it would help a ton (codec, config, etc)

Interestingly, only crashing on Linux and Mac, not windows.  And most but not all appear to be Hangouts (one meet.jit.si).  One is repeated a few hours apart, which implies a repeatable action caused it (screensharing, or some resolution change triggering a reconfig, etc).  Looks more like a reconfig in-call than a start-of-stream config (screensharing), but that's very much a guess.

Comment 3

[Emil Pasca [:emilpasca]]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0

This is 100% reproducible for us, using the following steps:

[Steps to reproduce]:
1. Login to "hangouts.google.com".
2. Click the "Video Call" button to start a video call.
3. Click on the "three dots" button at the top right part of the screen and choose the "Share Screen" option.
4. Choose the "Full Screen" option and observe.

[Crash Reports]:
Firefox: https://crash-stats.mozilla.com/report/index/e496541a-c166-4916-bf71-fee0b1180306
Nightly: https://crash-stats.mozilla.com/report/index/42e2abcb-d89b-4c69-8b8f-423cd0180306

[Notes]:
- The issue is not reproducible if you receive a shared screen through the video call session.
- Attached about:support info: https://pastebin.mozilla.org/9079241

Comment 4

[Dan Minor [:dminor]]

(In reply to Emil Pasca [:emilpasca], Desktop Engineering QA from comment #3)
> User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101
> Firefox/58.0
> 
> This is 100% reproducible for us, using the following steps:
> 
> [Steps to reproduce]:
> 1. Login to "hangouts.google.com".
> 2. Click the "Video Call" button to start a video call.
> 3. Click on the "three dots" button at the top right part of the screen and
> choose the "Share Screen" option.
> 4. Choose the "Full Screen" option and observe.
> 
> [Crash Reports]:
> Firefox:
> https://crash-stats.mozilla.com/report/index/e496541a-c166-4916-bf71-
> fee0b1180306
> Nightly:
> https://crash-stats.mozilla.com/report/index/42e2abcb-d89b-4c69-8b8f-
> 423cd0180306
> 
> [Notes]:
> - The issue is not reproducible if you receive a shared screen through the
> video call session.
> - Attached about:support info: https://pastebin.mozilla.org/9079241

Hi Emil,

I'm having difficulty reproducing this on Firefox/60.0 on Linux. I was wondering if it would be possible for you to try again using Nightly? There have been a number of screensharing issues with hangouts and I wanted to see if you were possibly hitting one in 58 that has subsequently been fixed in 60 which would explain why this is not 100% reproducible for me. Thank you,

Dan

Comment 5

[Emil Pasca [:emilpasca]]

Hi Dan,

I've just retested the issue on Ubuntu 16.04 x64 with the latest Nightly (60.0a1-20180307220100) with the same STR and the tab crashes, here's the crash report:
https://crash-stats.mozilla.com/report/index/12d724c4-e1ba-48c4-8202-9608d0180308


I would like to mention that I've tested this on a lower-end machine, I've added the about:support info below if it helps.
If there's anything else you would like me to test in order to help investigate this further, do let me know.


Application Basics
------------------

Name: Firefox
Version: 60.0a1
Build ID: 20180307220100
Update Channel: nightly
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
OS: Linux 4.4.0-116-generic
Multiprocess Windows: 1/1 (Enabled by default)
Web Content Processes: 3/4
Stylo: content = true (enabled by default), chrome = true (enabled by default)
Enterprise Policies: Inactive
Google Key: Found
Mozilla Location Service Key: Found
Safe Mode: false

Crash Reports for the Last 3 Days
---------------------------------

Report ID: bp-12d724c4-e1ba-48c4-8202-9608d0180308
Submitted: 1 minute ago

All Crash Reports

Nightly Features
----------------

Name: Activity Stream
Version: 2018.03.01.1281-6a7c8294
ID: [e-mail]

Name: Application Update Service Helper
Version: 2.0
ID: [e-mail]

Name: Firefox Screenshots
Version: 30.0.0
ID: [e-mail]

Name: Follow-on Search Telemetry
Version: 0.9.6
ID: [e-mail]

Name: Form Autofill
Version: 1.0
ID: [e-mail]

Name: Photon onboarding
Version: 1.0
ID: [e-mail]

Name: Pocket
Version: 1.0.5
ID: [e-mail]

Name: Presentation
Version: 1.0.0
ID: [e-mail]

Name: Web Compat
Version: 1.1
ID: [e-mail]

Name: WebCompat Reporter
Version: 1.0.0
ID: [e-mail]

Extensions
----------

Security Software
----------------- Type:

Type:

Type:

Graphics
--------

Features
Compositing: Basic
Asynchronous Pan/Zoom: wheel input enabled; scrollbar drag enabled; keyboard enabled; autoscroll enabled
WebGL 1 Driver WSI Info: GLX 1.4 GLX_VENDOR(client): Mesa Project and SGI GLX_VENDOR(server): SGI Extensions: GLX_ARB_create_context GLX_ARB_create_context_profile GLX_ARB_create_context_robustness GLX_ARB_fbconfig_float GLX_ARB_framebuffer_sRGB GLX_ARB_get_proc_address GLX_ARB_multisample GLX_EXT_import_context GLX_EXT_visual_info GLX_EXT_visual_rating GLX_EXT_fbconfig_packed_float G

Comment 6

[Dan Minor [:dminor]]

Hi Emil,

Thanks for your help on this. Unfortunately, I'm still having problems getting this to reproduce. I've tried on a few different systems without much luck.

I've added some extra debugging information and pushed to try here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cb676f46f1b442e7335372ced8c434df4b379cb
You can grab an archive of that version of firefox from here:
https://queue.taskcluster.net/v1/task/OqVVOK_CQyCFNnM7N8qowA/runs/0/artifacts/public/build/target.tar.bz2

I was wondering if you could try using that version and seeing if it reproduces for you? I've added the extra debugging as printf statements, so you should just be able to copy+paste the console output after it runs and attach it to the bug.

The other thing that would be helpful would be grabbing a copy of about:webrtc from before you start sharing your screen.

Thank you for your help!

Dan

Comment 7

[Emil Pasca [:emilpasca]]

I have retested this issue on the provided try-build from comment 6. The issue is still reproducible, below are the Browser Console error messages.

I've also tried to provide the WebRTC log from about:webrtc, however, after starting the debugger and stopping it, the about:webrtc page is empty, no stats are loaded in the page and also when navigating to "temp/WebRTC.log", the log is empty. Am I missing a step or something?

As a side note, the issue is reproducible also when using a meet call and attempt to share just a window, rather than the entire desktop.

Browser Console error messages on Ubuntu 16.04 x64 ------------------------

this is undefined  UTEventReporting.jsm:51
Content Security Policy: Ignoring “'unsafe-inline'” within script-src: ‘strict-dynamic’ specified (unknown)
Content Security Policy: Ignoring “https:” within script-src: ‘strict-dynamic’ specified (unknown)
Content Security Policy: Ignoring “http:” within script-src: ‘strict-dynamic’ specified (unknown)
WARNING!  rs=AL5CKSGKzmqFOQsi1kxEMltKH41GGPGj9Q:278:242
Using this console may allow attackers to impersonate you and steal your information using an attack called Self-XSS.
Do not enter or paste code that you do not understand.  rs=AL5CKSGKzmqFOQsi1kxEMltKH41GGPGj9Q:278:242
Attempt to set a forbidden header was denied: Connection  702790028-lcs_client_bin.js:99:385
Attempt to set a forbidden header was denied: Connection  702790028-lcs_client_bin.js:99:385
Content Security Policy: Ignoring ‘x-frame-options’ because of ‘frame-ancestors’ directive.  (unknown)
The character encoding of a framed document was not declared. The document may appear different if viewed without the document framing it.  hscv
URL.createObjectURL(MediaStream) is deprecated and will be removed soon.  webrtcUI.jsm:674:24
browser.ownerGlobal is null  ContentCrashHandlers.jsm:178



Browser Console error messages on Debian 4.9.0-4-amd64 ------------------------------


Content Security Policy: Ignoring “'unsafe-inline'” within script-src: ‘strict-dynamic’ specified (unknown)
Content Security Policy: Ignoring “https:” within script-src: ‘strict-dynamic’ specified (unknown)
Content Security Policy: Ignoring “http:” within script-src: ‘strict-dynamic’ specified (unknown)
Attempt to set a forbidden header was denied: Connection  702790028-lcs_client_bin.js:99:385
Attempt to set a forbidden header was denied: Connection  702790028-lcs_client_bin.js:99:385
Content Security Policy: Ignoring ‘x-frame-options’ because of ‘frame-ancestors’ directive.  (unknown)
WARNING!  rs=AL5CKSGKzmqFOQsi1kxEMltKH41GGPGj9Q:278:242
Using this console may allow attackers to impersonate you and steal your information using an attack called Self-XSS.
Do not enter or paste code that you do not understand.  rs=AL5CKSGKzmqFOQsi1kxEMltKH41GGPGj9Q:278:242
The character encoding of a framed document was not declared. The document may appear different if viewed without the document framing it.  hscv
URL.createObjectURL(MediaStream) is deprecated and will be removed soon.  webrtcUI.jsm:674:24
browser.ownerGlobal is null

Comment 8

[Dan Minor [:dminor]]

Hi Emil,

Sorry, I wasn't looking for the Browser Console output, but the Linux terminal output, so if you open the terminal on Ubuntu, run firefox from the command line, the output that you see there is what I'm looking for. I should have been more clear - I realize you probably get a lot more people asking for the browser console.

As for about:webrtc, it makes sense that would be empty. When I asked, I wasn't sure if another person had to be in the call for this to reproduce, but I've since learned that it will reproduce when you share the screen even if no one else is on the call.

This has crashed for me now, once on meet.google.com, and once on hangouts. I think the problem is we're returning an error from this function [1] but as you can see, there are lot of possible things that would cause an error there and without it crashing for me consistently it's hard for me to determine which one is the problem.

Thank you for your help,

Dan

[1] https://searchfox.org/mozilla-central/rev/a70da6775d5341a9cca86bf1572a5e3534909153/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc#290

Comment 9

[Dan Minor [:dminor]]

nevermind, finally consistently reproducing for me!

Comment 10

[Dan Minor [:dminor]]

The failure I'm seeing is here [1] while sharing a window on meet.google.com. It's checking aspect ratios. I'm guessing I had problems getting this to reproduce because I was choosing windows that had an aspect ratio which happened to result in working simulcast resolutions.

[1] https://searchfox.org/mozilla-central/rev/a70da6775d5341a9cca86bf1572a5e3534909153/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc#88

Comment 11

[Andreas Pehrson [:pehrsons]]

I think my (still local) patches on bug 1253499 would help with this. The ones attached to the bug don't handle simulcast layers, but I think I have some that do lying around. The way we choose resolutions for simulcast right now is... rudimentary. Something like `layer.width = width << layer.index` and likewise for height. So with an odd width or height your aspect ratio becomes off and we'll hit this assert.

Comment 12

[Tim Haloun]

Interesting about the aspect ratio.  That would explain why it more frequently triggers with windows than desktops.  I have been trying to change the whole system to disable simulcast while screencasting, without much success.  Screencast content is basically unviewable if pre-scaled.

Comment 13

[Dan Minor [:dminor]]

I'm not sure why we have our own code for choosing simulcast resolutions rather than using what is available in [1] which presumably is what is used by Chrome. The interesting bit there is that simulcast layers while screensharing are limited to 1 or 2 (if an experiment is enabled) in the version of the webrtc.org code we're currently using, presumably to avoid aspect ratio issues like we're seeing.

I'm planning to limit the number of simulcast streams to those which result in valid resolutions as a workaround for the current problem. Longer term, I think it would make sense to borrow what we can from [1] rather than maintaining our own algorithm which I don't think is getting us much in this case.

[1] https://searchfox.org/mozilla-central/source/media/webrtc/trunk/webrtc/media/engine/simulcast.cc

Comment 14

[Dan Minor [:dminor]]

I investigated using the cricket::GetSimulcastConfig function I mentioned above. That will definitely work, but it offers a much more restrictive model than what we currently have. For instance, the only allow a fixed set of simulcast resolutions [1] and I had to add a row to that table to make our unit tests work with two streams at a 50x50 resolution. That also throws away a lot of the flexibility that we allow during negotation, e.g. a lot of the fields at [2] would end up being ignored. The advantage of using GetSimulcastConfig is that we would no longer be generating configurations that cause assertions elsewhere in the video encoding stack. We'd also match the behaviour of Chrome which might be helpful for developers. The checks in the vp8 encoder limit how far we can diverge from the webrtc.org simulcast behaviour anyway.

The other approach I've looked into is to limit the number of layers to prevent problems. If I only allow a single simulcast layer when screensharing, and limit the number of layers so that we don't attempt to create a smaller resolution layer from a layer with odd dimensions then that fixes the problems with screensharing simulcast on meet.google.com. Interestingly, just limiting to a single simulcast layer when screensharing is not sufficient to fix the problem as it will crash when switching back from screenshare to camera, it looks like the request is made for camera but with the dimensions from the previous window being shared. And, as Andreas pointed out, allowing window sharing with more than one layer will crash if someone resizes the window to have an odd dimension.

I don't have the background on the decisions made during our implementation of simulcast to have a strong opinion on the best way ahead here. Maybe there's a better fix that I haven't considered. :jib, :perhsons, thoughts on this? Thanks!

[1] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/media/webrtc/trunk/webrtc/media/engine/simulcast.cc#43
[2] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/media/webrtc/signaling/src/common/EncodingConstraints.h#14

Comment 15

[Andreas Pehrson [:pehrsons]]

Unfortunately I don't know much of our simulcast impl history. Jesup maybe?

Comment 16

[Nils Ohlmeier [:drno]]

Since Byron has spend quite some time on our simulcast implementation he might be able to contribute information here.

Comment 17

[Byron Campen [:bwc]]

(In reply to Dan Minor [:dminor] from comment #10)
> The failure I'm seeing is here [1] while sharing a window on
> meet.google.com. It's checking aspect ratios. I'm guessing I had problems
> getting this to reproduce because I was choosing windows that had an aspect
> ratio which happened to result in working simulcast resolutions.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> a70da6775d5341a9cca86bf1572a5e3534909153/media/webrtc/trunk/webrtc/modules/
> video_coding/codecs/vp8/vp8_impl.cc#88

Back when I first worked on this stuff, I had code that would ensure that we preserved the aspect ratio exactly, but it appears to have been commented out during the 57 update:

https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#612

Comment 18

[Dan Minor [:dminor]]

I think the best way ahead here is to switch to using cricket::GetSimulcastConfig. We lose some flexibility but gain safety and consistency with Chrome. I'll clean up and post my patches for review shortly.

Comment 19

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

wfm, though I'd like to know what jesup thinks.

Comment 20

[Dan Minor [:dminor]]

(In reply to Dan Minor [:dminor] from comment #18)
> I think the best way ahead here is to switch to using
> cricket::GetSimulcastConfig. We lose some flexibility but gain safety and
> consistency with Chrome. I'll clean up and post my patches for review
> shortly.

I discussed this again with Nils and it looks like we are looking for something that we can easily uplift to 60 for hangouts/meet, so I'm going to go with the workaround instead. I'll file a separate bug for switching to cricket::GetSimulcastConfig.

Comment 21

[Dan Minor [:dminor]]

(In reply to Byron Campen [:bwc] from comment #17)
> (In reply to Dan Minor [:dminor] from comment #10)
> > The failure I'm seeing is here [1] while sharing a window on
> > meet.google.com. It's checking aspect ratios. I'm guessing I had problems
> > getting this to reproduce because I was choosing windows that had an aspect
> > ratio which happened to result in working simulcast resolutions.
> > 
> > [1]
> > https://searchfox.org/mozilla-central/rev/
> > a70da6775d5341a9cca86bf1572a5e3534909153/media/webrtc/trunk/webrtc/modules/
> > video_coding/codecs/vp8/vp8_impl.cc#88
> 
> Back when I first worked on this stuff, I had code that would ensure that we
> preserved the aspect ratio exactly, but it appears to have been commented
> out during the 57 update:
> 
> https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/
> media-conduit/VideoConduit.cpp#612

The comments in VideoConduit suggest that each layer must both have the same aspect ratio and also be exactly half the size of the previous layer. As far as I can tell, this size constraint is not actually enforced by branch 57. I did a quick experiment hard coding some layers to be a third of the previous layer's size and that worked fine on meet.google.com.

That said, the webrtc.org code in branch 57 always generates layers that are half the size of the preceding one when not screensharing [1] and when we update to branch 64, a check will be added that each layer is half the size of the previous one in the vp8 encoder [2], so I don't think re-enabling this code gains us much at this point.

Resizing a shared window does not trigger another call to CreateEncoderStreams so using ConstrainPreservingAspectRatioExact won't prevent us from crashing if the user resizes the window to an odd dimension after it is shared.

[1] https://searchfox.org/mozilla-central/source/media/webrtc/trunk/webrtc/media/engine/simulcast.cc#234
[2] https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc?q=ValidSimu&sq=package:chromium&l=90

Comment 22

[Dan Minor [:dminor]]

This is how I reproduce this:
1. Login to hangouts.google.com using your mozilla ldap account.
2. Go to meet.google.com, start a meeting. (I was not able to use meet from Firefox unless I was authenticated using my ldap account.)
3. Select presentation, share a window.
4. Resize the window a few times.
5. Stop presenting and switch back to camera.

Unpatched Firefox will crash at 3 or 4 depending upon the resolution of the window being shared. I also hit crashes at 5 with an earlier version of the patch.

Comment 23

[Dan Minor [:dminor]]

Attachment: Bug 1432793 - Force screensharing simulcast to one layer and stop generating layers once an odd width and height are found;

This limits screensharing simulcast to a single layer. When window sharing, our
source video can have arbitrary dimensions. If one of those dimensions ends up
being odd, the aspect ratio of the smaller layer will not match the aspect ratio
of the odd sized layer, causing a runtime assertion failure and crash.

It is not sufficient to prevent the creation of odd sized layers in
CreateEncoderStreams because the user can resize the window while it is being
shared, which will cause a fatal assertion prior to the streams being recreated.

When switching back from window sharing to camera, a call to
CreateEncoderStreams will occur with resolutions matching the dimensions of
the window that was just shared. To prevent a crash, this also adds a check
which prevents the creation of layers with odd resolutions.

Looking at cricket::GetSimulcastConfig for the version of webrtc.org in tree,
the number of simulcast layers is limited to one, or two if a field experiment
is enabled. That code also limits resolutions at which screensharing is allowed
as well as the number of layers that can be created for each resolution, and
ensures that each layer is exactly half the size of the layer above.

Adding these new constraints to CreateEncoderStreams makes us more consistent
with what the webrtc.org code would do when creating streams, which should
help to avoid more assertion failures in the future. Long term, I believe we
should just switch to using cricket::GetSimulcastConfig.

Review commit: https://reviewboard.mozilla.org/r/233592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233592/

Comment 24

[Dan Minor [:dminor]]

I'll write a mochitest for this as well, but I thought it made sense to get the patch reviewed since it is holding up hangouts/meet and is easily verifiable without a test.

Comment 25

[Dan Minor [:dminor]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe0d25e0c7c5e06a2caf8c1bbf6ff872a9d40cbc

Comment 26

[Byron Campen [:bwc]]

Comment on attachment 8964841 [details]
Bug 1432793 - Force screensharing simulcast to one layer and stop generating layers once an odd width and height are found;

https://reviewboard.mozilla.org/r/233592/#review239244

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:589
(Diff revision 1)
> +  // Disallow odd width and height, they will cause aspect ratio checks to
> +  // fail in the webrtc.org code. We can hit transient states after window
> +  // sharing ends where odd resolutions are requested for the camera.
> +  for (size_t i = 1; i < streamCount; ++i) {
> +    int ith_width = width >> (i - 1);
> +    int ith_height = height >> (i - 1);
> +    if (ith_width % 2 == 1 || ith_height % 2 == 1) {
> +      streamCount = i;
> +      break;
> +    }
> +  }

See if you can use this:

https://searchfox.org/mozilla-central/source/mfbt/MathAlgorithms.h#328

If you have to monkey around with include paths in the build system, it probably isn't worth doing, since (hopefully) this code won't need to stick around for long.

Comment 27

[Dan Minor [:dminor]]

Attachment: Bug 1432793 - Add mochitest for odd simulcast resolutions;

This creates a simulcast stream with an odd resolution. This previously would
have caused a runtime assertion failure and crash.

Review commit: https://reviewboard.mozilla.org/r/233754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233754/

Comment 28

[Dan Minor [:dminor]]

Attachment: Bug 1432793 - Add gtests for simulcast screenshare and odd resolutions;

Review commit: https://reviewboard.mozilla.org/r/233756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233756/

Comment 29

[Dan Minor [:dminor]]

Comment on attachment 8964841 [details]
Bug 1432793 - Force screensharing simulcast to one layer and stop generating layers once an odd width and height are found;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233592/diff/1-2/

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/847e9f622eb7
Force screensharing simulcast to one layer and stop generating layers once an odd width and height are found; r=bwc

Comment 31

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60a9050fb70f
Backed out changeset 847e9f622eb7 for build bustages on VideoConduit.cpp. CLOSED TREE

Comment 32

[Byron Campen [:bwc]]

Comment on attachment 8965011 [details]
Bug 1432793 - Add mochitest for odd simulcast resolutions;

https://reviewboard.mozilla.org/r/233754/#review239464

Comment 33

[Byron Campen [:bwc]]

Comment on attachment 8965012 [details]
Bug 1432793 - Add gtests for simulcast screenshare and odd resolutions;

https://reviewboard.mozilla.org/r/233756/#review239462

r+ with fixes

::: media/webrtc/signaling/gtest/videoconduit_unittests.cpp:745
(Diff revision 1)
> +  EncodingConstraints constraints;
> +  VideoCodecConfig::SimulcastEncoding encoding;
> +  VideoCodecConfig::SimulcastEncoding encoding2;
> +  VideoCodecConfig::SimulcastEncoding encoding3;
> +  encoding2.constraints.scaleDownBy = 2;
> +  encoding2.constraints.scaleDownBy = 4;

encoding3?

::: media/webrtc/signaling/gtest/videoconduit_unittests.cpp:768
(Diff revision 1)
> +  EncodingConstraints constraints;
> +  VideoCodecConfig::SimulcastEncoding encoding;
> +  VideoCodecConfig::SimulcastEncoding encoding2;
> +  VideoCodecConfig::SimulcastEncoding encoding3;
> +  encoding2.constraints.scaleDownBy = 2;
> +  encoding2.constraints.scaleDownBy = 4;

encoding3?

Comment 34

[Dan Minor [:dminor]]

Try run with build fix and tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e771797133e81efcec824f6231dcae9b16baf948

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c5a16ba81b7
Force screensharing simulcast to one layer and stop generating layers once an odd width and height are found; r=bwc
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1929e2b77a9
Add mochitest for odd simulcast resolutions; r=bwc
https://hg.mozilla.org/integration/mozilla-inbound/rev/620a4eba9389
Add gtests for simulcast screenshare and odd resolutions; r=bwc

Comment 36

[Dan Minor [:dminor]]

Comment on attachment 8964841 [details]
Bug 1432793 - Force screensharing simulcast to one layer and stop generating layers once an odd width and height are found;

Approval Request Comment
[Feature/Bug causing the regression]:
I believe this has been a problem since we first implemented simulcast (Bug 1210170).
[User impact if declined]:
This is blocking hangouts/meet supporting Firefox and causes a browser crash.
[Is this code covered by automated tests?]:
New tests have been added in the other patches on this bug.
[Has the fix been verified in Nightly?]:
My push looks fine on inbound but has not yet been merged into central.
[Needs manual test from QE? If yes, steps to reproduce]: 
Yes, see comment 22.
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
This only impacts simulcast with odd resolutions. Since cameras and screens typically have even resolutions, this should only change behaviour when window sharing. Since window sharing simulcast was basically completely broken anyway, this should not make things worse.
[String changes made/needed]:

Comment 37

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/5c5a16ba81b7
https://hg.mozilla.org/mozilla-central/rev/f1929e2b77a9
https://hg.mozilla.org/mozilla-central/rev/620a4eba9389

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8964841 [details]
Bug 1432793 - Force screensharing simulcast to one layer and stop generating layers once an odd width and height are found;

Fix needed for Hangouts/Meet. Approved for 60.0b11.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/70140bcf230d
https://hg.mozilla.org/releases/mozilla-beta/rev/e493c1e01d24
https://hg.mozilla.org/releases/mozilla-beta/rev/bb3cfe7f354d

Comment 40

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Attachment: crash

The issue is still reproducible on Windows 10 (x64) and macOS 10.12 using latest Nightly (2018-04-11) and Firefox 60.0b11 using the following steps:
Precondition:
Make sure a folder is opened in background and not minimized (it will be needed later)

Steps:
1. Start Firefox.
2. Login to hangouts.google.com.
3. Go to meet.google.com (you have to be logged in with mozilla ldap account, or else it will say that the page is unsupported on Firefox).
4. Click on Start a new meeting.
5. Click on Start Meeting.
6. Click on Present Now and select a Window.
7. The folder from step 1 will be selected for screenshare.
8. After the screenshare started, minimize the folder.

On Ubuntu 16.04 (x64) this issue is not reproducible.

Here is a crash log from macOS: https://crash-stats.mozilla.com/report/index/04613a3b-74fd-4a96-8870-f21120180412#tab-details

Hi Dan, can you have a look into this?
Should I file a new bug for this issue?

I attached a screencast with the issue too.

Comment 41

[Dan Minor [:dminor]]

Hi Sascha,

I'll look into this right away. Since this doesn't affect Ubuntu, I suspect this is a separate, but closely related bug. The crash signature just means that reconfiguring the encoder has failed but unfortunately gives no information on what went wrong. Thank you for catching this.

Comment 42

[Dan Minor [:dminor]]

I filed Bug 1453740 for this. A minimized window has a resolution of 1x1 pixels, which causes a separate check in the vp8 codec to fail.

Comment 43

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Since this bug is still reproducible, and it depends on Bug 1453740, I cannot verify it until it is fixed.

Comment 44

[Catalin Sasca, Desktop Test Engineering [:csasca]]

This issue is no longer reproduced by following the steps mentioned in Comment 22 and Bug 1453740 was verified fixed.