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

Use buffer to gather frames #186

Merged
merged 6 commits into from
May 10, 2023

Conversation

wouter-heerwegh
Copy link
Contributor

Made some changes to use a buffer instead of accessing the frame variable of the BackgroundFrameRead object. This makes sure you're not reading data from the frame variable while the BackgroundFrameRead object is writing data to it, thus causing less artifacts in the images.

@M4GNV5
Copy link
Collaborator

M4GNV5 commented May 8, 2023

I am not 100% sure about this change, because:

  • when not calling the BackgroundFrameRead.get_frame function, the BackgroundFrameRead.frames list will grow infinitly and consume a lot of memory.
  • when programming image recognition based control systems the image recognition shall always be applied to the latest image, dropping some frames when the recognition takes longer is fine in that case.

We could think about:

  • using the Queue class with a max-sized queue and non-blocking puts instead of list
  • still provide the old BackgroundFrameRead.frame field for grabbing the latest frame
  • maybe create a function for retrieving the latest frame, rather than accessing a field (i.e. have both a get_queued_frame and a get_latest_frame function)

@wouter-heerwegh
Copy link
Contributor Author

I made some changes based upon your suggestions. I need to test these latest changes, because my main issue was that the frame variable is being accessed while being written to, and this gave me images with a lot of artifacts.

I think it would be best to not access the variable directly, but to use the get_frame function which is wrapped with a lock so that there is no way to write and read data at the same time.

Will let you know when I have done some further testing.

Copy link
Collaborator

@M4GNV5 M4GNV5 left a comment

Choose a reason for hiding this comment

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

On second thought using the Queue class might not have been the best recommendation.
I picked it for its thread safety features.

Its main downside is its behaviour on put calls when the queue is full (i.e. it drops the new items). What we probably want is to drop the oldest item (i.e. the oldest frame).
The documentation suggests the collections.dequeue class, which is also threadsafe, but drops oldest items instead of the newest ones.

If you want you can change the code to use collections.dequeue or just fix the issues i commented on. I am fine with both solutions.

djitellopy/tello.py Outdated Show resolved Hide resolved
djitellopy/tello.py Outdated Show resolved Hide resolved
djitellopy/tello.py Outdated Show resolved Hide resolved
@wouter-heerwegh
Copy link
Contributor Author

I made the changes you requested. I also made it so that you can decide whether to use the queue or not at all. By default, the queue is not used.

I still have to test the changes, will do that tomorrow.

Copy link
Collaborator

@M4GNV5 M4GNV5 left a comment

Choose a reason for hiding this comment

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

looks very good now!

@wouter-heerwegh
Copy link
Contributor Author

Made some changes, to make it work. It works on our Tello Talent now. In our case the queue version works the best. I'm polling the frame at a rate of 60hz, so when there is some delay, I will catch up over time.

1 thing to note (I think this is already in the documentation), enough time between polling of the frame is needed, otherwise the frame will only very occasionally be locked by the BackgroundFrameRead itself. This causes the same frame to be shown or a big delay.

@wouter-heerwegh
Copy link
Contributor Author

Also, when using the queued version, accessing the frame variable will occasionally return None (when no frame is available). This has to be checked with is ant not ==

djitellopy/tello.py Outdated Show resolved Hide resolved
djitellopy/tello.py Show resolved Hide resolved
djitellopy/tello.py Outdated Show resolved Hide resolved
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.

2 participants