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

Pass the width and height when requesting camera images #9683

Merged
merged 16 commits into from
Aug 11, 2021
Merged

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Aug 2, 2021

Proposed change

Pass the width and height when requesting camera images

Requires home-assistant/core#53835

Testing:

  • Tested with Chrome with network throttling to fast 3g. Cameras are now usable
  • Tested with mobile device with poor service. Cameras are now usable

Additional improvement would be to not update cameras that are not inside the viewport, but that is outside the scope of this PR because even with this change there are timeouts on slow 3g (#9690).

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

src/data/camera.ts Outdated Show resolved Hide resolved
src/data/camera.ts Outdated Show resolved Hide resolved
src/data/camera.ts Outdated Show resolved Hide resolved
src/data/camera.ts Outdated Show resolved Hide resolved
balloob
balloob previously approved these changes Aug 10, 2021
hass: HomeAssistant,
entityId: string,
...args: unknown[]
...args: any[]
Copy link
Member Author

Choose a reason for hiding this comment

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

It is working with width and height but I had to change the typing here. Not sure this it the best solution.

Copy link
Member

Choose a reason for hiding this comment

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

I tried locally but couldn't figure it out. It's all good.

@balloob balloob merged commit b36e342 into dev Aug 11, 2021
@balloob balloob deleted the camera_image_size branch August 11, 2021 04:42
@balloob
Copy link
Member

balloob commented Aug 11, 2021

One thing I did just realize after I merged it (ofc), we will cache by entity ID and not by width/height. So there might be cases where a user caches a smaller image and so the new bigger location will serve from the cache.

@bdraco
Copy link
Member Author

bdraco commented Aug 11, 2021

I'm likely missing something here. If I load two windows, I see them get different size images.
Screen_Shot_2021-08-11_at_8_16_16_AM

@bdraco
Copy link
Member Author

bdraco commented Aug 11, 2021

I tried loading the smaller one first, then refreshing the bigger one but still got the larger image. I'm missing something

Screen_Shot_2021-08-11_at_8_20_00_AM

@bramkragten
Copy link
Member

We don't cache these files, otherwise the image would never move

@bdraco
Copy link
Member Author

bdraco commented Aug 11, 2021

Thanks. I thought it might have something to do with #9690 merged in to my test branch, but that didn't make sense.

@bramkragten
Copy link
Member

Well we do cache within the session, but not in another screen/window/refresh, and we also only cache for 9 seconds...

So it could be an issue if you have the same entity on your page twice in different sizes I guess...

@bramkragten
Copy link
Member

To fix that, we should replace entityId in timeCachePromiseFunc with an itemKey and include both the entity id and size in that.

@bdraco
Copy link
Member Author

bdraco commented Aug 11, 2021

I'll open something for that shortly

@bdraco
Copy link
Member Author

bdraco commented Aug 11, 2021

To fix that, we should replace entityId in timeCachePromiseFunc with an itemKey and include both the entity id and size in that.

#9778

@github-actions github-actions bot locked and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants