Bounds are not always correctly set when setting twice a MediaStream to a HTMLMediaElement #15445
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
EWS run on previous version of this PR (hash 47f8e63)
|
youennf
force-pushed
the
eng/Bounds-are-not-always-correctly-set-when-setting-twice-a-MediaStream-to-a-HTMLMediaElement
branch
from
47f8e63
to
13857e3
Compare
|
EWS run on previous version of this PR (hash 13857e3)
|
youennf
force-pushed
the
eng/Bounds-are-not-always-correctly-set-when-setting-twice-a-MediaStream-to-a-HTMLMediaElement
branch
from
13857e3
to
a30bbda
Compare
|
EWS run on previous version of this PR (hash a30bbda)
|
youennf
force-pushed
the
eng/Bounds-are-not-always-correctly-set-when-setting-twice-a-MediaStream-to-a-HTMLMediaElement
branch
from
a30bbda
to
a7fdc07
Compare
|
EWS run on previous version of this PR (hash a7fdc07) |
youennf
force-pushed
the
eng/Bounds-are-not-always-correctly-set-when-setting-twice-a-MediaStream-to-a-HTMLMediaElement
branch
from
a7fdc07
to
5a9668c
Compare
|
EWS run on previous version of this PR (hash 5a9668c) |
jernoble
approved these changes
Jun 30, 2023
| m_rootLayer.get().bounds = CGRectMake(0, 0, size.width(), size.height()); | ||
| auto bounds = CGRectMake(0, 0, size.width(), size.height()); | ||
| m_rootLayer.get().bounds = bounds; | ||
| m_rootLayer.get().position = { bounds.size.width / 2, bounds.size.height / 2 }; |
There was a problem hiding this comment.
The defaults for anchorPoint will be different on different platforms, so it may be worthwhile to set these to { 0.5, 0.5 } here. Also keep in mind that position is relative to the layer's parent's bounds. Shouldn't be a problem here if the m_rootLayer's parent always has the same bounds as it does.
There was a problem hiding this comment.
And same for m_sabpleBufferDisplayLayer, whose anchorPoint initialization was removed above.
There was a problem hiding this comment.
I will add back anchorPoint to m_sampleBufferDisplayLayer. Are you suggesting I do the same for the root layer even though we are not doing any transform to that layer?
Also which platform is not using { 0.5, 0.5 }?
youennf
force-pushed
the
eng/Bounds-are-not-always-correctly-set-when-setting-twice-a-MediaStream-to-a-HTMLMediaElement
branch
from
5a9668c
to
3bc7118
Compare
|
EWS run on current version of this PR (hash 3bc7118) |
…to a HTMLMediaElement https://bugs.webkit.org/show_bug.cgi?id=258723 rdar://110753228 Reviewed by Jer Noble. After double layer hosting, we were relying on MediaPlayerPrivateMediaStreamAVFObjC::setVideoInlineSizeFenced to be called so that the AVSampleBufferDisplayLayer bound would be set. This works fine initially but not when resetting srcObject to a new MediaStream. In that case, we create a new SampleBufferDisplayLayer but the bounds may not be correctly initialized. We are making sure to initialize both root and AVSampleBufferDisplayLayer bounds and positions. They might still not be correctly set due to rotation. This patch is thus refactoring the code so that LocalSampleBufferDisplayLayer will handle rotation itself. This is feasible since it receives VideoFrames. We remove sending affine transforms updates from WebProcess to GPUProcess. Instead, whenever we receive a VideoFrame in LocalSampleBufferDisplayLayer, we compute the affine transform to use. If there is a need for a new affine transform, we update the bounds of the AVSampleBufferDisplayLayer, which triggers creating a new one currently. This simplifies things as now, only the AVSampleBufferDisplayLayer has to get an affine transform and switch width and height depending on frame rotation. To reduce flicker, we introduce m_isReconfiguring which will ensure we do not enqueue new video frames to an AVSampleBufferDisplayLayer that will be removed soon. We are also using inline video size instead of presentation size if possible to reduce reflowing. There are still some undesired flicker/reflowing happening when rotation changes since both the media element bounds may change and the rotation. And these steps are not done at once. * LayoutTests/fast/mediastream/video-srcObject-set-twice-expected.html: Added. * LayoutTests/fast/mediastream/video-srcObject-set-twice.html: Added. * Source/WebCore/platform/graphics/avfoundation/SampleBufferDisplayLayer.h: * Source/WebCore/platform/graphics/avfoundation/objc/LocalSampleBufferDisplayLayer.h: * Source/WebCore/platform/graphics/avfoundation/objc/LocalSampleBufferDisplayLayer.mm: (WebCore::LocalSampleBufferDisplayLayer::initialize): (WebCore::LocalSampleBufferDisplayLayer::updateBoundsAndPosition): (WebCore::LocalSampleBufferDisplayLayer::updateSampleLayerBoundsAndPosition): (WebCore::videoTransformationMatrix): (WebCore::LocalSampleBufferDisplayLayer::enqueueVideoFrame): (WebCore::LocalSampleBufferDisplayLayer::enqueueBufferInternal): (WebCore::LocalSampleBufferDisplayLayer::requestNotificationWhenReadyForVideoData): (WebCore::LocalSampleBufferDisplayLayer::updateRootLayerAffineTransform): Deleted. (WebCore::LocalSampleBufferDisplayLayer::updateAffineTransform): Deleted. (WebCore::LocalSampleBufferDisplayLayer::setRootLayerBoundsAndPositions): Deleted. (WebCore::LocalSampleBufferDisplayLayer::updateRootLayerBoundsAndPosition): Deleted. (WebCore::LocalSampleBufferDisplayLayer::enqueueBuffer): Deleted. * Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h: * Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm: (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::enqueueVideoFrame): (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::layersAreInitialized): (WebCore::videoTransformationMatrix): (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::rootLayerBoundsDidChange): (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::setVideoInlineSizeFenced): * Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.cpp: (WebKit::RemoteSampleBufferDisplayLayer::updateBoundsAndPosition): (WebKit::RemoteSampleBufferDisplayLayer::enqueueVideoFrame): (WebKit::RemoteSampleBufferDisplayLayer::updateAffineTransform): Deleted. * Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.h: * Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.messages.in: * Source/WebKit/WebProcess/GPU/webrtc/SampleBufferDisplayLayer.cpp: (WebKit::SampleBufferDisplayLayer::updateBoundsAndPosition): (WebKit::SampleBufferDisplayLayer::updateAffineTransform): Deleted. * Source/WebKit/WebProcess/GPU/webrtc/SampleBufferDisplayLayer.h: Canonical link: https://commits.webkit.org/265708@main
webkit-commit-queue
force-pushed
the
eng/Bounds-are-not-always-correctly-set-when-setting-twice-a-MediaStream-to-a-HTMLMediaElement
branch
from
3bc7118
to
38b1396
Compare
|
Committed 265708@main (38b1396): https://commits.webkit.org/265708@main Reviewed commits have been landed. Closing PR #15445 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
38b1396
3bc7118