Skip to content

add option to set content_type in camera.generic to support 'svg cameras'#8188

Merged
balloob merged 3 commits into
home-assistant:devfrom
azogue:svg-camera
Jun 25, 2017
Merged

add option to set content_type in camera.generic to support 'svg cameras'#8188
balloob merged 3 commits into
home-assistant:devfrom
azogue:svg-camera

Conversation

@azogue
Copy link
Copy Markdown
Member

@azogue azogue commented Jun 24, 2017

Description:

Add customisable content_type to the generic camera to be able to get snapshots and stream a dynamic svg file.

Related issue (if applicable): fixes #8162

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2887

Example entry for configuration.yaml (if applicable):

camera:
  - platform: generic
    name: Weather
    still_image_url: https://www.yr.no/place/Norway/Oslo/Oslo/Oslo/meteogram.svg
    content_type: 'image/svg+xml'

Checklist:

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

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@mention-bot
Copy link
Copy Markdown

@azogue, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @pvizeli to be potential reviewers.

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Jun 24, 2017

That need unittests...

@azogue
Copy link
Copy Markdown
Member Author

azogue commented Jun 24, 2017

@pvizeli, I'll look to that.

I wasn't sure about this approach to get svg cameras running, do you approve? (I thought also of a new camera platform for svg files as direct approach)

@azogue
Copy link
Copy Markdown
Member Author

azogue commented Jun 24, 2017

@pvizeli, I'm thinking that this new option to set the content_type is also desirable for the local_file camera, don't you? There it can be done with the path_file extension also... what do you think?

@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 24, 2017

This won't work as when we're streaming, we're converting the jpeg files to mjpeg.

@azogue
Copy link
Copy Markdown
Member Author

azogue commented Jun 25, 2017

Hi @balloob,

I'm sorry to disagree with you, but in the generation of the video stream no conversion is performed, but frames are added sequentially, separated with an identifier. Each frame has its mime type, which is currently set to 'image/jpeg' (although it works for cameras that return a PNG image!, I think the browser renders them in spite of the error)

Before proposing the PR, this change I tested in iOS Safari, Safari, Firefox and Chrome for Mac, and works perfectly for both the still image and the video stream.

When making me doubt, I even made a mini component derived from the local_file camera that supports a list of files and changes them sequentially (to test these SVG dynamic cams with a high refresh rate, up to 5fps). It works perfectly for SVG files with just the change in the mime type.

I hope you reconsider the statement that it won't work.
Another thing is if, for this type of cameras would have to create a new platform for SVGs, which would allow both urls and local files, instead of modifying the generic and local_file cameras.

@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 25, 2017

I must admit that I didn't believe you but tested it locally and it works 👍 . Learned something new today.

@azogue
Copy link
Copy Markdown
Member Author

azogue commented Jun 25, 2017

Thank you very much for reconsider, @balloob!

I'm no video expert, but when I have the time I like to explore and try. I myself had this problem of SVG cameras at the beginning of using Home Assistant, and I solved it by converting the SVGs into PNGs on the origin server in order to be able to display some plots in HA from a custom component I have for a current meter. Now I will use this direct approach.

So, what do you think about the local_file camera? Should I include the new parameter also there? or set the content_type there automatically with the file extension (if it is a PNG or SVG file)?

@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 25, 2017

Guessing is the way to go: example code

@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 25, 2017

Going to merge this, you can open another PR for local file.

@balloob balloob merged commit 4ca5ed2 into home-assistant:dev Jun 25, 2017
@balloob balloob mentioned this pull request Jul 1, 2017
dethpickle pushed a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017
…ras' (home-assistant#8188)

* add custom content_type to support 'generic svg cameras'

* add unittest to check content_type for svg generic camera

* Tweak tests
@home-assistant home-assistant locked and limited conversation to collaborators Oct 20, 2017
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.

Error loading image for camera component using svg image

6 participants