search

hanselb / HyperionScreenCap

Screen capture program for Hyperion ambilight
MIT License
60 stars 28 forks source link

Protobuffer and misc improvements #8

Closed RickDB closed 8 years ago

RickDB commented 8 years ago
hanselb commented 8 years ago

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 commented 8 years ago

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 :+1: 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:

https://github.com/RickDB/HyperionScreenCap/commit/147b75062085e70bce68c4318ac26e8aec7f47c9#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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@hanselb Still alive? :+1:

hanselb commented 8 years ago

Hey @brindosch yeah I'm still kicking it lol I have been super busy with life :stuck_out_tongue_closed_eyes: 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 :+1:

brindosch commented 8 years ago

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

hanselb commented 8 years ago

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.

RickDB commented 8 years ago

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.
  2. Not implemented yet but do agree that we should add it as should have no downside as we keep sending.
hanselb commented 8 years ago

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. :smile: https://github.com/hanselb/HyperionScreenCap/issues/9 https://github.com/hanselb/HyperionScreenCap/issues/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.