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

Calling video comparison to improve the security strength #2291

Merged
merged 5 commits into from
Mar 15, 2022

Conversation

oscar-davids
Copy link
Contributor

@oscar-davids oscar-davids commented Feb 25, 2022

Calling video comparison functions on results received from different Os

Specific updates (required)

How did you test each of these updates (required)

  • Call video comparison function in chooseResults().
  • Passed Unit Test.

Does this pull request close any open issues?

Fix #315

Checklist:

Copy link
Contributor

@leszko leszko left a 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:

  1. Traffic-wise, we add downloading the whole segment. I guess it's ok, but just want to double-check it.
  2. One optimization would be to check first pHash and only if pHash matches, then download the segment. But not sure we need it because we actually expect pHash to match in most cases.
  3. 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.
  4. I didn't check if ffmpeg.CompareVideoByBuffer() works fine and the implication on having some false-negative results.

server/push_test.go Outdated Show resolved Hide resolved
server/push_test.go Show resolved Hide resolved
@yondonfu yondonfu requested a review from AlexKordic March 1, 2022 15:27
@oscar-davids
Copy link
Contributor Author

  1. Traffic-wise, we add downloading the whole segment. I guess it's ok, but just want to double-check it.

Yeah, there is additional traffic for downloading the random n-th segment.

  1. One optimization would be to check first pHash and only if pHash matches, then download the segment. But not sure we need it because we actually expect pHash to match in most cases.
  2. 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 prefer the second optimization path because I think it is a little late to check validity at that time here with the third way.

  1. I didn't check if ffmpeg.CompareVideoByBuffer() works fine and the implication on having some false-negative results.

For H264 encoding, it showed 100% accuracy in GPU vs CPU cross-comparison. I think we need to re-evaluate the H265 codec.

Copy link
Contributor

@AlexKordic AlexKordic left a comment

Choose a reason for hiding this comment

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

Well Done!

Copy link
Contributor

@leszko leszko left a 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!

@oscar-davids oscar-davids merged commit 10705d2 into master Mar 15, 2022
@oscar-davids oscar-davids deleted the oc/checkvideo branch March 15, 2022 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants