-
Notifications
You must be signed in to change notification settings - Fork 177
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
Calling video comparison to improve the security strength #2291
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks, good! Good work @oscar-davids
Added 2 minor comments. Other than that, here are some comments things to consider:
- Traffic-wise, we add downloading the whole segment. I guess it's ok, but just want to double-check it.
- One optimization would be to check first
pHash
and only ifpHash
matches, then download the segment. But not sure we need it because we actually expectpHash
to match in most cases. - One other optimization could be that we actually download the segment from untrusted O twice. One for the fast verification and later to download the result here. Again, maybe we don't need to change it right now.
- I didn't check if
ffmpeg.CompareVideoByBuffer()
works fine and the implication on having some false-negative results.
Yeah, there is additional traffic for downloading the random n-th segment.
I prefer the second optimization path because I think it is a little late to check validity at that time here with the third way.
For H264 encoding, it showed 100% accuracy in GPU vs CPU cross-comparison. I think we need to re-evaluate the H265 codec. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thanks for adding the unit test!
Calling video comparison functions on results received from different Os
Specific updates (required)
How did you test each of these updates (required)
chooseResults()
.Does this pull request close any open issues?
Fix #315
Checklist:
make
runs successfully./test.sh
pass