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

Protobuffer and misc improvements #8

Merged
merged 9 commits into from
Jul 13, 2016
Merged

Protobuffer and misc improvements #8

merged 9 commits into from
Jul 13, 2016

Conversation

RickDB
Copy link
Contributor

@RickDB RickDB commented Apr 18, 2016

  • Removed JSON support, ProtoBuffer is now the default method to keep it simple and to follow Hyperion standards.
  • Removed timer_tick solution for capture and replaced it with loop, capture interval is now no longer needed but can help with slower systems.
  • Re-added missing parts of ProtoClient.
  • Implemented reconnect support.
  • Set DirectX PresentInterval to PresentInterval.Immediate.
  • Cleaned up code.

RickDB added 4 commits April 18, 2016 11:04
…t simple and to follow Hyperion standards.

Removed timer_tick solution for capture and replaced it with loop, capture interval is now no longer needed but can help with slower systems.
Re-added missing parts of ProtoClient.
Implemented reconnect support.
Set DirectX PresentInterval to PresentInterval.Immediate.
Cleaned up code.
Removed remaining Timer references.
@hanselb
Copy link
Owner

hanselb commented Apr 19, 2016

Hi @RickDB,
Thank you for taking the time to contribute.

I checked your commit locally and there is a memory issue. It is smaller than the last pull request but still present (about +1mb every 5-8 seconds). Also, the image is shown sideways on the LEDs because of the way the ProtoClient is handling the pixel array. I already have some pixel manipulation on my code which is cleaner and uses less resources. Maybe we can add it to the SendImage method in the ProtoClient.

I would also like to talk about your approach of using a loop and having to clear the images/priorities. I believe that simply setting a duration for the image to hyperion removes the need to have to clear the image when we are done sending information, also removing the chance of an image getting stuck in the LEDs because the application crashed.

I want to create an amazing application together, so I would really love to collaborate instead of step on each other's toes.

I look forward to your response.

@RickDB
Copy link
Contributor Author

RickDB commented Apr 20, 2016

SendImage could indeed use further improving, it's from on our AtmoLight implementation and that part is pretty old and wasn't made by our team.
We don't notice any memory leaks but does get cleared after video playback there so that is most likely the reason (although never grows above 50MB there) :) , if you want to update that with your approach that would be great 👍
In the meantime will see if I can make a quick patch for that and add it to the PR.

We do need to flip the image because when it's converted into a memory stream it comes out mirrored and also strip the headers, with the current PR code it does match 1:1 with what's on the screen here atm.

Adding a fixed duration in the sendimage request is a good idea so that in case of unexpected interrupts during loop it will not lock out any other applications with lower priorities, can also read the protobuffer request reply in case of errors but that does slow down processing a bit:

RickDB@147b750#diff-ab01d62e631701e756335156709669dfR224

Want to make this amazing app together as well and glad to help out where ever I can, Hyperion forum will be up soon (~1-2 weeks) which has dedicated areas for 3rd party apps and such so that way we can probably share ideas easier and also get user feedback / feature requests :)

@RickDB
Copy link
Contributor Author

RickDB commented Apr 20, 2016

Pushed potential fix for memory leak, forgot to add back the 2 dispose calls.
Could re-use the Surface but not sure if there will be any performance gain.

@RickDB
Copy link
Contributor Author

RickDB commented May 3, 2016

Hi @hanselb ,

Forum and new wiki went live so can discuss some ideas there if you want :)

https://hyperion-project.org/

Also been using this PR version for some time and seeing no memory leaks anymore so looks fixed.

@brindosch
Copy link

@hanselb
Still alive? 👍

@hanselb
Copy link
Owner

hanselb commented Jun 15, 2016

Hey @brindosch yeah I'm still kicking it lol
I have been super busy with life 😝 but I am going to try to do some work on this soon.
@RickDB I see that you are working on your fork, I will reach out to you before I start working on mine again to merge.
Talk to you soon 👍

@brindosch
Copy link

Glad to hear from you, we had the fear you where lost somewhere :)

@hanselb
Copy link
Owner

hanselb commented Jul 13, 2016

Hi @RickDB I hope you are doing well.

So I am going to merge this pull request and then continue my implementation. There are a couple of things I want to go over.

  1. The last time I used this branch the LEDs displayed the image sideways. Have you tested any other capture software? Does your LEDs match the orientation and position of the HyperCon preview? I didn't get any complaints from other users with my code, so this could be an isolated issue with your setup.
  2. On this branch I still see the same implementation with clearing the priority etc, instead of just using a set duration for the image sent so that it doesn't stay stuck on the screen if something fails. Is there any specific reason why you want to do it this way? If there are none, I would like to switch that approach and have you test it on your environment.

I am going to start working on my local environment, but will not push until we get clarification on those points.

@hanselb hanselb merged commit 78c1bd4 into hanselb:master Jul 13, 2016
@RickDB
Copy link
Contributor Author

RickDB commented Jul 14, 2016

Hi @hanselb doing fine and glad to see you working on it again :)

  1. Not sure about the cause but did get a few users to test this PR version and couldn't find any errors with that (clockwise setups for most), however did find a few new ones:
  • If Windows 10 DPI scaling is > 100% it will not capture and report an DX error (no surface to capture)
  • Multi monitor setup is iffy and looks related to SlimDX, can't always capture correctly even if monitor index is set to the right one.
  1. Not implemented yet but do agree that we should add it as should have no downside as we keep sending.

@hanselb
Copy link
Owner

hanselb commented Jul 14, 2016

Hi @RickDB
My setup is counter clockwise, so maybe that's why its apparent on mine. I just pushed some changes to the image processing that should remove the need to flip the image.

I added the new issues you mentioned and will start troubleshooting them soon. If you have any ideas or new bugs just add them as issues. I am working my way through them. 😄
#9
#10

I will remove the ClearPriority method calls and work on the image duration. I am thinking on maybe calculating how fast the software is capturing the screen to report FPS and to set the duration to just above the frame duration. Maybe do something similar with the capture interval.

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.

3 participants