-
Notifications
You must be signed in to change notification settings - Fork 12
Add frame by frame compression and test this compression - Nikita Ostapenko #30
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
Conversation
-Some nuances are not in this version such as the logger middleware/advanced http options
-I am not sure about what model is so I am leaving it as the concatenation of the operating system name and its version -Type needs to be changed later to be assigned propertly, for now only uses 3 as this is for the cli which only runs on desktops
-Currently unable to gather data (coming soon) -Will not leave sessions (will be implemented with data gathering)
…llow CamelCase from the original GrpcClient.cs
-Currently only able to collect color frames
YiqinZhao
left a comment
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.
Some changes are needed. Overall, you did great! I made some changes on code styling, documentations, and formatting.
python/client/GrpcClient.py
Outdated
| @@ -0,0 +1,76 @@ | |||
| import grpc | |||
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.
We should always add a leading docstring for Python scripts.
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.
Please check https://github.com/catmajor/ARFlow/pull/1/files for some changes I made to this file. Overall, you did great. I made some changes on code styling, documentations, and formatting.
python/client/GrpcClient.py
Outdated
| class GrpcClient: | ||
| def __init__(self, url): | ||
| self.channel = grpc.insecure_channel(url) | ||
| async def create_session_async(self, name: str, device: Device, save_path: str = "") -> CreateSessionResponse: |
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.
Change return type in the function definition to Awaitable[CreateSessionResponse].
python/client/GrpcClient.py
Outdated
| session_metadata=SessionMetadata(name=name, save_path=save_path), | ||
| device=device | ||
| ) | ||
| response: Awaitable[CreateSessionResponse] = ARFlowServiceStub(self.channel).CreateSession(request) |
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.
ARFlowServiceStub can be reused?
python/client/GrpcClient.py
Outdated
| ) | ||
| response: Awaitable[CreateSessionResponse] = ARFlowServiceStub(self.channel).CreateSession(request) | ||
| return response | ||
| async def delete_session_async(self, session_id: str) -> DeleteSessionResponse: |
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.
Need an empty line between L35 and L34.
| @@ -0,0 +1,136 @@ | |||
|
|
|||
|
|
|||
| from GrpcClient import GrpcClient | |||
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.
Please follow my changes in https://github.com/catmajor/ARFlow/pull/1/files to update this and other files as well.
new: code style and docstring improvements.
merge individual frame work and tests into my main branch
Frame by Frame compressionWe discussed adding frame by frame compression to ARFlow as H264 compression creates a challenge in integrating chunked and compressed byte processing into existing one frame one display results. Here they are. I have incorporated a decently modified version of one of Vedant Saran's tests. I have left in print statements despite ruff's complaints as they are not used for debugging, but rather in the application itself to display messages and UIs. Overall ResultsUncompressed frames were by far the worst, in HD the FPS cannot even get above 5. The method requires lots of processing power and bandwidth. PNG frames were slightly better, the HD version nearly doubled the FPS to 7, but the overall effect compared to the original H264 compression is also quite bad. However, unlike the raw frames, this method is actually bottlenecked by the processing power required to compress individual images into PNG format. This is visible through the difference between memory use between the SD and HD versions where the amount of memory actually went down due to the main bottleneck being created by compression speed rather than bandwidth use. JPG saw the closest results to the extreme efficiency seen with H264 compression. There was a ~90% reduction in bandwidth and the frame rate actually stayed near 30 FPS for both SD and HD tests. It is important to note that JPG compression still requires heavy CPU use and for the HD version this occupied nearly 40% of my total cpu's output. |
YiqinZhao
left a comment
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.
Please see the comments for some minor formatting issues.
|
|
||
| async def __manage_sessions(self): | ||
| while True: | ||
| print("Available Sessions:") |
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.
This Ruff (T201) error needs to be addressed. Try adding either # ruff: noqa: T201 to the top of the file or using the following statements to disable the error messages on a code block:
# ruff: noqa: T201
print("This is allowed") # Ruff will ignore T201 here
print("This is also allowed")
# ruff: enable
| return response | ||
|
|
||
| def close(self): | ||
| """Close the channel.""" |
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.
Address this Ruff error.
cleanup small ruff formatting issue
YiqinZhao
left a comment
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
Create basic CLI client for ARFlow in python
Provide a variety of APIs, some of which closely resemble those found in the Unity Client to provide functions for clients