sailfishos / sailfish-browser

Sailfish Browser
https://github.com/sailfishos/sailfish-browser/wiki/Sailfish-Browser-wiki
Mozilla Public License 2.0
305 stars 86 forks source link

Fix WebRTC video feed discolouration #1064

Open llewelld opened 1 month ago

llewelld commented 1 month ago

When streaming video using WebRTC the image is discoloured. Post-processing using GIMP suggests it's the red and blue channels which are being switched around.

It's not clear whether this is an issue that's happening in gecko or elsewhere in the stack, but since it's not happening with an ESR 78, that would suggest the problem is in gecko-dev.

gecko-webrtc-channel-switch

krnlyng commented 1 month ago

I checked this a while ago in a different context and i had these findings:

The issue is most likely in gecko-camera: https://github.com/sailfishos/gecko-camera

There is a DroidMediaBufferYCbCr ycbcrTemplate which gets set up and passed to DroidCameraYCbCrFrame::map() and eventually used to describe the buffer layout but it seems some parts of it are wrong.

Usually the DroidMediaBufferYCbCr is supposed to be initialized by droid_media_buffer_lock_ycbcr. But in this case it seems that this structure is read from somewhere else and doesn't match the buffer because it's not gotten from droid_media_buffer_lock_ycbcr.

Thaodan commented 1 month ago

Assuming for a second that the issue isn't in the hybris based camera stack if the issue would make sense to check if similar reports exist for that version.

A test case that doesn't need a camera device would be great to evade errors outside of Gecko itself.

llewelld commented 1 month ago

Thanks for the helpful comments. I've not had a chance to look at this properly, but it appears the point at which gecko and gecko-camera meet is in video_capture_sfos.cc, the code for which is all in Patch 0081.

Looking at this it's not immediately clear where the ycbcrTemplate template gets set up.

A quick check with the debugger shows that rtc::scoped_refptr<I420BufferInterface> ToI420() is getting called with ycbcr->chromaStep set to 2, so that libyuv::NV12ToI420() conversion is triggered.

Might it be possible to extract a test case from this? I agree, this would be really helpful to have.

krnlyng commented 1 month ago

Usually it is passed to droid_media_buffer_lock_ycbcr as a pointer which will then fill in the values. Like here: https://github.com/sailfishos/gecko-camera/blob/master/plugins/droid/droid-common.cpp#L70

But in the camera case droid_media_buffer_lock_ycbcr is never called, but droid_media_camera_recording_frame_get_data https://github.com/sailfishos/gecko-camera/blob/master/plugins/droid/droid-camera.cpp#L617 is used to get the buffer data instead.

The ycbcrTemplate is set up here: https://github.com/sailfishos/gecko-camera/blob/master/plugins/droid/droid-camera.cpp#L705 which seems to not match the underlying buffer.