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

scan: Not scanning every picture #1065

Merged
merged 3 commits into from
Feb 3, 2022
Merged

scan: Not scanning every picture #1065

merged 3 commits into from
Feb 3, 2022

Conversation

M123-dev
Copy link
Member

@M123-dev M123-dev commented Feb 2, 2022

No description provided.

@github-actions github-actions bot added 🤳 MLKit 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… labels Feb 2, 2022
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @M123-dev, I have a minor request change, feel free to ignore it and merge.

Comment on lines 167 to 170
if (frameCounter < 10) {
frameCounter++;
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I would start immediately: here the very first scan is after 1/3 seconds.
  2. I would use a static const with an explicit name, like static const int _SKIPPED_FRAMES = 10;

Copy link
Contributor

@jasmeet0817 jasmeet0817 Feb 2, 2022

Choose a reason for hiding this comment

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

How about

if (framCounter++ % 10 != 0) {
  return;
}

and if we have fears of it reaching MAX_INT then:

if (framCounter++ % 10 != 0) {
  return;
}
frameCounter=1;

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with @monsieurtanuki. Otherwise LG

Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://stackoverflow.com/a/50429767/14517119

In Dart 2 int is limited to 64 bit

I think we're safe before we reach MAX_INT then ;)

@jasmeet0817 Your modulo suggestion does not work because once we decide to work we don't know how much time it will take - if it takes more than 1/3 seconds it's not good.

@M123-dev I see that you also use a bool, isBusy. I suggest that you use a 3-state bool? instead:

  • isBusy == false means that you're ready to compute immediately
  • isBusy == true means that you're working
  • isBusy == null means that you're done with working and that you'll wait 10 frames before working again

Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://stackoverflow.com/a/50429767/14517119

In Dart 2 int is limited to 64 bit

I think we're safe before we reach MAX_INT then ;)

@jasmeet0817 Your modulo suggestion does not work because once we decide to work we don't know how much time it will take - if it takes more than 1/3 seconds it's not good.

@M123-dev I see that you also use a bool, isBusy. I suggest that you use a 3-state bool? instead:

  • isBusy == false means that you're ready to compute immediately
  • isBusy == true means that you're working
  • isBusy == null means that you're done with working and that you'll wait 10 frames before working again

An enum would be better if we want more than 2 states:)

General questions:

  1. If the processing takes more than 1/3rd sec, do we want to process next frame directly or wait 9 frames and then process.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm against waiting until the latest I'd done and then counting, this could lead to very long waiting times

Copy link
Contributor

Choose a reason for hiding this comment

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

@M123-dev Both @jasmeet0817 and I agreed on most of your code: please just add a TODO about this frame skipping algorithm and merge.

Copy link
Member Author

@M123-dev M123-dev Feb 3, 2022

Choose a reason for hiding this comment

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

Just added the const var for the skipped frames and the todo

@M123-dev M123-dev merged commit 7d8e9c2 into develop Feb 3, 2022
@M123-dev M123-dev deleted the scanner-fps branch February 3, 2022 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤳 MLKit 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion…
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants