Unity-Technologies / com.unity.webrtc

WebRTC package for Unity
Other
738 stars 185 forks source link

fix: Fix flip vertical video image for received video #981

Closed karasusan closed 9 months ago

karasusan commented 9 months ago

Fix a bug in this PR.

aet commented 9 months ago

the default behavior for VideoStreamTrack changed too. Now i'm supposed to pass CopyTextureHelper.VerticalFlipCopy, is it on purpose?

karasusan commented 9 months ago

@aet The previous PR changes the flag false. It looks passing true is right. https://github.com/Unity-Technologies/com.unity.webrtc/pull/971/files#diff-f4a6f4c4bccd5c3f082e2ee65fc6785d7c1af35098d1d68374a1ae2955ba7f25R182

MaximKurbanov commented 9 months ago

@aet The previous PR changes the flag false. It looks passing true is right. https://github.com/Unity-Technologies/com.unity.webrtc/pull/971/files#diff-f4a6f4c4bccd5c3f082e2ee65fc6785d7c1af35098d1d68374a1ae2955ba7f25R182

I also changed the Sender, now the Sender sends the image without flip. And upon receipt, a flip is not required. If we put flip, we will get a vertically flipped image. If you receive a normal image from a camera(webcamera) without flip, then you also do not need to flip it when sending. Else if you received a fliped image from a camera, then you need to flip it to normal and send it. So the receiver doesn't need flip.

972

aet commented 9 months ago

@MaximKurbanov Hum, I don't use any of the webcam stuff but just receiving external video tracks and rendering to textures and sending them forward (includes the received video track), in both scenarios the default behavior changed? I think the intent of the new CopyTexture parameter is good, to give flexibility to rare cases like the smart glasses you were using.

The default should be something that works for majority of the users, being backwards compatible is a plus. I'm ok explicitly adding the flip flags on my tracks, but seems like extra step for majority of the users.

aet commented 9 months ago

Sorry, I was talking about public VideoStreamTrack(Texture texture, bool needFlip = true) changing into VideoStreamTrack(Texture texture, CopyTexture copyTexture = null) that is a different case from NeedReceivedVideoFlipVertically

MaximKurbanov commented 9 months ago

The default should be something that works for majority of the users, being backwards compatible is a plus. I'm ok explicitly adding the flip flags on my tracks, but seems like extra step for majority of the users.

I agree with you.

MaximKurbanov commented 9 months ago

My recommendation for optimization is to set NeedReceivedVideoFlipVertically to false. And use "copyTexture: Graphics.Blit" in the VideoStreamTrack constructor.

karasusan commented 9 months ago

@MaximKurbanov I agree with you about suggestion of the performance improvement. However, it is needed to support browser and unity at the same time.