fix race condition in i420 video converter leading to video artefacts

[EmrysMyrddin]

Description

Fixes a race condition in the i420 video converter leading to artefacts when used in concurrently.

A way to see it in action is to create 2 x264 video encoder for the same video track with a video source using i422 or i444 color space. This triggers a bug where we don't convert the entire image, we only resample it, probably to avoid unecessary memory allocation. But when multiple encoders uses this converter at the same time, it will resample multiple time with the same original sample ration, leading to weird artefacts.

There is probably 2 way to fix this:

• add locks to avoid concurrent modifications
• make a copy when doing destructive operation

I have implemented the later one in this PR since adding a lock would be difficult, necessitating to be aware of it everywhere the frame is used. I think the simplicity to have a very local fix covers the additional memory and cpu consumption.

[codecov[bot]]

Codecov Report

Merging #418 (c90490e) into master (e32fc1b) will increase coverage by 0.08%. The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master     #418      +/-   ##
==========================================
+ Coverage   47.23%   47.32%   +0.08%     
==========================================
  Files          67       67              
  Lines        4456     4463       +7     
==========================================
+ Hits         2105     2112       +7     
  Misses       2226     2226              
  Partials      125      125              

Impacted Files	Coverage Δ
pkg/io/video/convert.go	66.66% <66.66%> (-0.33%)	:arrow_down:
pkg/io/video/convert_cgo.go	68.65% <100.00%> (+4.24%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e32fc1b...c90490e. Read the comment docs.

[EmrysMyrddin]

This has a inpact on performances (+50% on 480p, +100% in 1080p). It still far better than the FrameBuffer.StoreCopy which converts to RGBA when we enable frame copy.

Perhaps someone has a better idea than my naive copy implementation ? perhaps something written in C ? since we already massively rely on C in this package.

[at-wat]

Perhaps someone has a better idea than my naive copy implementation ? perhaps something written in C ? since we already massively rely on C in this package.

I wrote several CGO version of heavy functions in this package, but left pure Go version and make them automatically switched by cgo build tag. See https://github.com/pion/mediadevices/blob/master/pkg/frame/yuv_nocgo.go https://github.com/pion/mediadevices/blob/master/pkg/frame/yuv_cgo.go for examples.

[EmrysMyrddin]

After some though, I have tried an other approach, which I find cleaner: I don't copy image to avoid modification, I have refactored the converters to make them side effect free and not relying on pointers to give back the result of the conversion.

I'm much happier of this version, and the performance impact is far more acceptable:

name                  old time/op  new time/op  delta
ToI420/480p/I444-12    121µs ± 1%   169µs ± 0%  +39.17%  (p=0.008 n=5+5)
ToI420/480p/I422-12   70.3µs ± 1%  82.1µs ± 1%  +16.79%  (p=0.008 n=5+5)
ToI420/480p/I420-12   9.06ns ± 3%  9.26ns ± 1%     ~     (p=0.151 n=5+5)
ToI420/480p/RGBA-12    435µs ± 0%   480µs ± 2%  +10.46%  (p=0.008 n=5+5)
ToI420/1080p/I444-12   705µs ± 1%  1006µs ± 1%  +42.70%  (p=0.008 n=5+5)
ToI420/1080p/I422-12   405µs ± 2%   489µs ± 1%  +20.72%  (p=0.008 n=5+5)
ToI420/1080p/I420-12  9.01ns ± 0%  8.62ns ± 0%   -4.30%  (p=0.008 n=5+5)
ToI420/1080p/RGBA-12  2.72ms ± 0%  3.06ms ± 2%  +12.24%  (p=0.008 n=5+5)