Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REGRESSION(r294104): [GStreamer] getUserMedia broken #629

Merged
merged 0 commits into from May 16, 2022

Conversation

philn
Copy link
Member

@philn philn commented May 15, 2022

5e80996

REGRESSION(r294104): [GStreamer] getUserMedia broken
https://bugs.webkit.org/show_bug.cgi?id=240420

Patch by Philippe Normand <pnormand@igalia.com > on 2022-05-16
Reviewed by Xabier Rodriguez-Calvar.

The converter handling logic was modified in order to fix getUserMedia negotiated with raw
video and also getDisplayMedia which is always raw video and thus doesn't require decoding.

This patch also introduces a small optimization, reconfiguration is now done once only,
after setting size and framerate. Before this patch it was done twice, so the pipeline was a
taking more time to produce the first frame.

* platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:
(WebCore::GStreamerVideoCaptureSource::~GStreamerVideoCaptureSource):
(WebCore::GStreamerVideoCaptureSource::settingsDidChange): Trigger capturer reconfiguration after setting both size and framerate.
(WebCore::GStreamerVideoCaptureSource::startProducingData): Ditto.
* platform/mediastream/gstreamer/GStreamerVideoCapturer.cpp:
(WebCore::GStreamerVideoCapturer::createConverter): Do not decode display capture streams, these are always raw anyway.
(WebCore::GStreamerVideoCapturer::setSize): Delay reconfiguration.
(WebCore::GStreamerVideoCapturer::setFrameRate): Ditto.
(WebCore::GStreamerVideoCapturer::reconfigure): Keep track of compatible video format. This
is needed to workaround an issue in pipewiresrc caps negotiation.
(WebCore::GStreamerVideoCapturer::adjustVideoSrcMIMEType): Renamed to reconfigure().
* platform/mediastream/gstreamer/GStreamerVideoCapturer.h:

Canonical link: https://commits.webkit.org/250587@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294229 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@philn philn self-assigned this May 15, 2022
@philn philn added Platform Portability improvements and other general platform improvements not driven directly by site bugs. WebKit Nightly Build labels May 15, 2022
Copy link
Contributor

@calvaris calvaris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I have a question, though. Maybe I misunderstood something but it looks to me that we are forcing cameras to provide only video/x-raw and from what I've seen in cameras they either provide raw for low resolutions or if they provide higher resolutions, sometimes it's only a few fps. What do you think about allowing direct x-264 for example? From what I know the problem is USB bandwidth and it is cheaper to get the encoded video from the camera even when we have to decode it before sending it to the screen. I guess we would not have to decode it and re-encode it to send it on the wire...

@philn
Copy link
Member Author

philn commented May 16, 2022

LGTM but I have a question, though. Maybe I misunderstood something but it looks to me that we are forcing cameras to provide only video/x-raw and from what I've seen in cameras they either provide raw for low resolutions or if they provide higher resolutions, sometimes it's only a few fps. What do you think about allowing direct x-264 for example? From what I know the problem is USB bandwidth and it is cheaper to get the encoded video from the camera even when we have to decode it before sending it to the screen. I guess we would not have to decode it and re-encode it to send it on the wire...

On the paper it sounds like a good idea. Thibault and I actually made experiments a couple years ago with camerabin. The problem is that the video frame observers expect raw frames most of the time, so we need to decode either way. The exception is that when we know the stream goes directly to webrtcbin we could avoid decoding... On embedded platforms it might be needed actually, maybe an improvement for later!

@philn philn marked this pull request as ready for review May 16, 2022 11:33
@philn philn added the merge-queue Applied to send a pull request to merge-queue label May 16, 2022
@philn
Copy link
Member Author

philn commented May 16, 2022

Also I think h264 codec is not so common anymore in WebRTC, vp8, vp9 are more common nowadays. av1 will also become more prominent in the future.

@webkit-early-warning-system
Copy link
Collaborator

Committed r294229 (250587@main): https://commits.webkit.org/250587@main

Reviewed commits have been landed. Closing PR #629 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label May 16, 2022
@philn philn deleted the eng/240420 branch May 16, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform Portability improvements and other general platform improvements not driven directly by site bugs.
Projects
None yet
3 participants