1502172 - Crash in video capture on Win 10

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

Looks like some Win 10 users are experiencing crashes when trying to access their camera.

Comment 1

[Nils Ohlmeier [:drno]]

jib can you please have a look or dispatch to someone else?

Comment 2

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

Looks like a long-term low-volume crasher [1]. Did the recent ___/ activate a trigger?

These are in a RecvStartCapture() lambda, which I suppose might be some timing issue on our end, but I think it's more likely an OS issue, given it's a Windows OS call that succeeds with bad data [2]:

    AM_MEDIA_TYPE *pmt = NULL;

    hr = streamConfig->GetStreamCaps(
                                    windowsCapability.directShowCapabilityIndex,
                                    &pmt, reinterpret_cast<BYTE*> (&caps));
    if (!FAILED(hr))
    {
->      if (pmt->formattype == FORMAT_VideoInfo2)

Andreas, WDYT?

[1] https://crash-stats.mozilla.com/signature/?signature=webrtc%3A%3Avideocapturemodule%3A%3AVideoCaptureDS%3A%3ASetCameraOutput&date=%3E%3D2018-04-26T17%3A00%3A56.000Z&date=%3C2018-10-26T17%3A00%3A56.000Z#graphs
[2] https://hg.mozilla.org/releases/mozilla-release/annotate/c9ed11ae5c79df3dcb69075e1c9da0317d1ecb1b/media/webrtc/trunk/webrtc/modules/video_capture/windows/video_capture_ds.cc#l281

Comment 3

[Andreas Pehrson [:pehrsons]]

It's hard to say much from these reports, but as far as proofs go, we also have a couple of reports with crash address 0x0 (most are 0x2c). The 0x0 ones crash at [1] instead, like [2].


[1] https://hg.mozilla.org/releases/mozilla-release/annotate/150c70d39363b57ea4e92fbbd900b338f09177d6/media/webrtc/trunk/webrtc/modules/video_capture/windows/video_capture_ds.cc#l265
[2] https://crash-stats.mozilla.com/report/index/dedda384-362f-48fc-b8bf-b575c0180923


It looks like we already have this on file in bug 876884.

The 0x0 crashes seem to support jesup's comment there:

> _outputCapturePin must be null...
> 
> VideoCaptureDS::Init() doesn't check the result of GetOutputPin() (or
> GetInputPin()), which may be the root cause of the crash.
> 
> This would be an upstream bug in webrtc.org code


He's referring to [3], which doesn't check that the pin is not null, unlike [4], which is another call to the same method.

[3] https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/media/webrtc/trunk/webrtc/modules/video_capture/windows/video_capture_ds.cc#112
[4] https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/media/webrtc/trunk/webrtc/modules/video_capture/windows/video_capture_ds.cc#392

I haven't found official docs on GetInputPin or GetOutputPin, but we should probably try what jesup suggested and guard against null pins.

Comment 4

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] from comment #3)
> I haven't found official docs on GetInputPin or GetOutputPin, but we should
> probably try what jesup suggested and guard against null pins.

Ah, because they're part of webrtc.org ... [5]

Yes, clearly there's no guarantee the pins will be non-null.

[5] https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/media/webrtc/trunk/webrtc/modules/video_capture/windows/help_functions_ds.cc#35,68

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1502172 - Guard against null pins in directshow video capture code. r?dminor

Comment 6

[Andreas Pehrson [:pehrsons]]

My plan here is to land locally first, then verify on crash-stats once it has been on release a while. If there's an improvement I'll upstream.

Comment 7

[Dan Minor [:dminor]]

In branch 64, WEBRTC_TRACE has been replaced by RTC_LOG. This will probably land first anyway, but I'll leave a note here just in case.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b7884f2b670
Guard against null pins in directshow video capture code. r=dminor

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4b7884f2b670

Comment 11

[Andreas Pehrson [:pehrsons]]

This is still ongoing, but I think I have found another problem. Filing a follow-up.