1807215 - Webcam window gets cropped after disabling/enabling it on Facebook calls

Status: VERIFIED FIXED

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

Description

[Ardelean Oana, Desktop QA [:oardelean]]

Attachment: after re-enabling

Notes

• This does not reproduce in Chrome. Please see the attached image for more details.

Found in

• Firefox 110.0a1;

Affected versions

• Firefox 110.0a1;
• Firefox 109.0b6;
• Firefox 108.0.1;

Tested platforms

• Ubuntu 22;
• macOS 12;

Affected platforms

• Ubuntu 22;
• macOS 12;

Preconditions

• Have at least 2 people in the same Facebook video call.
• Webcam is enabled.

Steps to reproduce

1. Disable the webcam from the facebook camera button at the bottom of the call window.
2. Enable the webcam.

Expected result

• Webcam window resumes the same position before disabling it.

Actual result

• Webcam window gets cropped to the right.

Regression range

• Will look for one ASAP.

Comment 1

[Ardelean Oana, Desktop QA [:oardelean]]

Mozregression points to:

• last good build: 2022-07-10
• first bad build: 2022-07-11
• pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5c2e208ed1b682d0db1602552566d8400f40f9c6&tochange=419c7e63a9f4cd8efd4727d84c3a44e97dcf3f3c
• Regressed by: bug 1757637

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1757637

:padenot, since you are the author of the regressor, bug 1757637, could you take a look?

For more information, please visit auto_nag documentation.

Comment 3

[Andreas Pehrson [:pehrsons]]

Fwiw this is where a video MediaStreamTrack triggers HTMLMediaElement::Invalidate which in turn triggers "resize" which is what Facebook should be reacting on.

Since the regressor made changes to videoWidth and videoHeight I assume this is related to the timing of "resize". That said I'm not sure why there would be resizes happening from disabling/enabling the camera. Perhaps they do something heavier than just set enabled on the track.

Comment 4

[Paul Adenot (:padenot)]

Maybe we're updating those values too late for the "resize" event? I think what we need is an HTMLMediaElement log of this happening.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1757637

Comment 6

[Andreas Pehrson [:pehrsons]]

This is what videoWidth and videoHeight depends on:

gfx::IntSize HTMLVideoElement::GetVideoIntrinsicDimensions() {
  layers::ImageContainer* container = GetImageContainer();
  // Prefer the size of the container as it's more up to date.
  if (container && container->GetCurrentSize().width != 0) {
    // But adjust to the aspect ratio of the container.
    float dar = static_cast<float>(mMediaInfo.mVideo.mDisplay.width) /
                mMediaInfo.mVideo.mDisplay.height;
    gfx::IntSize size = container->GetCurrentSize();
    float imageDar = static_cast<float>(size.width) / size.height;
    return gfx::IntSize(int(size.width * (dar / imageDar)), size.height);
  }
  return mMediaInfo.mVideo.mDisplay;
}

There's a new dependency on container->GetCurrentSize() that came with bug 1757637. Prior to that bug they only depended on mMediaInfo.mVideo.mDisplay.

The timing dependency between "resize" and mMediaInfo.mVideo.mDisplay is simple -- "resize" will fire after setting mMediaInfo.mVideo.mDisplay.

The timing dependency between "resize" and container->GetCurrentSize() is not much worse, just a bit more indirection. ImageContainer::GetCurrentSize() uses mCurrentImages[0].mImage->GetSize(). Whereas the same sync path that dispatches "resize" reads VideoFrameContainer::mMainThreadState.mIntrinsicSizeChanged, which comes from mFrames[0].second.mFrame.GetIntrinsicSize() that is passed to SetCurrentFrames, but also does a thread-hop. This thread-hop is fine since it gets dispatched before the thread-hop that sets mMediaInfo.mVideo.mDisplay (and dispatches "resize") gets dispatched.

So race-wise "resize" vs videoWidth/videoHeight seems fine. Bug 1757637 should only have made it so we start returning the new resolution earlier, though because container->GetCurrentSize() and mMediaInfo.mVideo.mDisplay are not in sync (set on different threads), it could be that GetVideoIntrinsicDimensions above returns the wrong aspect ratio for a bit while waiting for that main-thread hop. Could be worth experimenting with this.

Comment 7

[Andreas Pehrson [:pehrsons]]

A recording: https://pernos.co/debug/ULKlitbh9QFmaD7h1zq8Ow/index.html

I went to a messenger group call room and turned the camera off and on a number of times. Non-opt so took a while.

See this view for when HTMLVideoElement::GetVideoIntrinsicDimensions returns 1x1 erroneously. mMediaInfo.mVideo.mDisplay is 1280x720 as expected.

The 1x1 comes from the black frame we render when a video track becomes disabled, here.

Comment 10

[Andreas Pehrson [:pehrsons]]

I think we should ask VideoFrameContainer for the current intrinsic size instead of ImageContainer for the current size. This is after all in HTMLVideoElement::GetVideoIntrinsicDimensions().

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1807215 - Modernize wpt MST-ME-disabled-video-is-black. r?padenot!

Comment 12

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1807215 - In wpt MST-ME-disabled-video-is-black clean up in a cleanup task. r?padenot!

Comment 13

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1807215 - In MST-ME-disabled-video-is-black correctly log the checked pixel. r?padenot!

Comment 14

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1807215 - In MST-ME-disabled-video-is-black expect to fill only the video element's intrinsic width and height. r?padenot!

Comment 15

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1807215 - Re-enable MST-ME-disabled-video-is-black. r?padenot!

Comment 16

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1807215 - Add wpt testcase. r?padenot!

Comment 17

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1807215 - Annotate VideoFrameContainer::mMutex. r?padenot!

Comment 18

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1807215 - Expose a VideoFrameContainer's current intrinsic size. r?padenot!

Comment 19

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1807215 - Make the intrinsic size arg in MediaDecoderOwner::Invalidate a const ref. r?padenot!

Comment 20

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1807215 - Simplify intrinsic size handling in VideoFrameContainer by removing the intrinsicSizeChanged flag. r?padenot!

Comment 21

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1807215 - Take video element intrinsic dimensions from VideoFrameContainer instead of ImageContainer. r?padenot!, r?alwu!

VideoFrameContainer already knows a frame's intrinsic size. The same intrinsic
size that drives media element invalidation.

ImageContainer just handles images and they don't have a notion of intrinsic
dimensions.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1759d7ac00e
Modernize wpt MST-ME-disabled-video-is-black. r=padenot
https://hg.mozilla.org/integration/autoland/rev/5eb150c1f051
In wpt MST-ME-disabled-video-is-black clean up in a cleanup task. r=padenot
https://hg.mozilla.org/integration/autoland/rev/263344e2a052
In MST-ME-disabled-video-is-black correctly log the checked pixel. r=padenot
https://hg.mozilla.org/integration/autoland/rev/2de3f8723542
In MST-ME-disabled-video-is-black expect to fill only the video element's intrinsic width and height. r=padenot
https://hg.mozilla.org/integration/autoland/rev/0c584facdd4d
Re-enable MST-ME-disabled-video-is-black. r=padenot
https://hg.mozilla.org/integration/autoland/rev/472b2b30e698
Add wpt testcase. r=padenot
https://hg.mozilla.org/integration/autoland/rev/d0057fb4ede0
Annotate VideoFrameContainer::mMutex. r=padenot
https://hg.mozilla.org/integration/autoland/rev/a2585761fea8
Expose a VideoFrameContainer's current intrinsic size. r=padenot
https://hg.mozilla.org/integration/autoland/rev/d444d0fb641b
Make the intrinsic size arg in MediaDecoderOwner::Invalidate a const ref. r=padenot
https://hg.mozilla.org/integration/autoland/rev/e149f608434e
Simplify intrinsic size handling in VideoFrameContainer by removing the intrinsicSizeChanged flag. r=padenot
https://hg.mozilla.org/integration/autoland/rev/6544f77c4e25
Take video element intrinsic dimensions from VideoFrameContainer instead of ImageContainer. r=padenot

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39329 for changes under testing/web-platform/tests

Comment 24

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/e1759d7ac00e
https://hg.mozilla.org/mozilla-central/rev/5eb150c1f051
https://hg.mozilla.org/mozilla-central/rev/263344e2a052
https://hg.mozilla.org/mozilla-central/rev/2de3f8723542
https://hg.mozilla.org/mozilla-central/rev/0c584facdd4d
https://hg.mozilla.org/mozilla-central/rev/472b2b30e698
https://hg.mozilla.org/mozilla-central/rev/d0057fb4ede0
https://hg.mozilla.org/mozilla-central/rev/a2585761fea8
https://hg.mozilla.org/mozilla-central/rev/d444d0fb641b
https://hg.mozilla.org/mozilla-central/rev/e149f608434e
https://hg.mozilla.org/mozilla-central/rev/6544f77c4e25

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.

Comment 26

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 27

[Ardelean Oana, Desktop QA [:oardelean]]

Reproducible on a 2023-04-01 Nightly build on macOS 12.
Verified as fixed on Firefox 113.0b2(build ID: 20230411180038) and Nightly 114.0a1(build ID: 20230412092401) on macOS 12, Windows 10, Ubuntu 22.