search

abrenoch / hyperion-android-grabber

Screen grabber for hyperion
MIT License
194 stars 33 forks source link

Feature/image quality #65

Closed abrenoch closed 6 years ago

abrenoch commented 6 years ago

Adds the ability to configure the quality of the grabbed image.

Decided to approach this using the LED count of the device hyperion is driving - those values are used to determine the optimal quality for the image. Since there are only specific values we can scale the screen by (while maintaining correct aspect ratio), this seems like the most user friendly way to pick the optimal setting.

There is a small change to the notification buttons in HyperionScreenService which I'm not sure how got mixed into this commit, but that is just adding the "EXIT" text to the strings xml & removing the extra button it is initialized with but never shows for some reason. So nothing that should impact the functionality of the app.

Closes #52!

Paulchen-Panther commented 6 years ago

I hope this fixes the OOM issue

abrenoch commented 6 years ago

That fix seems to be working for me @Paulchen-Panther... Nice work!

There is another error I see thrown (usually when using a higher image quality) which is the image buffer becoming inaccessible when reading the pixel data from it. It doesn't seem to cause any problems, but I would like to investigate sooner than later.

EDIT: Unfortunately it must have just been luck when I first tried it... Just got the same OOM again while working on the image buffer D:

abrenoch commented 6 years ago

I was still running into the OOMs with @Paulchen-Panther 's fix (despite my early enthusiasm hah). This fix sets a maximum response size to 1KB. The typical response size is 4 bytes, so if it is anything larger than that something fishy is up, so a 1KB maximum should be acceptable.

This seems to make things a lot more stable when performing the 'hammer the toggle button as fast as I can' test 😁

Paulchen-Panther commented 6 years ago

I used the variable usedSocket to counteract the socket closed error.

abrenoch commented 6 years ago

Unfortunately I'm still seeing the E/HyperionScreenService: Socket closed error happening with those changes, so I'm not sure it is having the intended effect. I honestly don't think it is a big deal anyway, since that error appears to be handled gracefully within HyperionScreenService and was unrelated to the OOM error (seemingly anyways).

Likewise with the image buffer error I mentioned earlier, I don't really think it is going to cause any problems since the error is being handled. The only concern I had there was it potentially not closing out an image from the buffer, but those errors appear to only be presented once the image is already closed (which is probably more of a synchronization issue).

I think if this PR looks alright as-is then we should merge and address any more fixes in separate PRs... I just don't want the scope of this one to start creeping if these issues existed already anyway!

EDIT: See my comment on #68, got the problem tracked down, but not resolved yet!

abrenoch commented 6 years ago

The OOM issue seems to have been resolved by using a BufferedByteArray when reading the socket response