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

Question about media platform. #9

Open
xannor opened this issue Mar 12, 2023 · 138 comments
Open

Question about media platform. #9

xannor opened this issue Mar 12, 2023 · 138 comments

Comments

@xannor
Copy link
Contributor

xannor commented Mar 12, 2023

Do you have any plans to build out a media platform using the search command? If you dont, or dont plan on doing it for a while, I might create a custom integration that wraps around the "built in" reolink and this api to implement one. I had built the original one in the fwesterberg custom integration, and has plans to build one in my own CI until your built in one surpassed it faster than I could work on it.

My idea is the media platform providing date sorted access to recordings and a custom sensor that provides info on the last recording for a channel. The only real drawback is that the rest api does not provide any trigger information other than a date/time, (but the client one does) and no potential thumbnails.

any thoughts?

@starkillerOG
Copy link
Owner

@xannor thank you very much for reaching out and your contributions to the projects on which this library is based on!

I was not planning on adding the media platform on the short term, but I would be more than happy to review and accept PRs (both in this library and in HomeAssistant) if you are willing to work on it.
I am more than happy to help out if you get stuck somewhere.

Your idea sounds good and was indeed already pretty much incoporated in the fwesterberg custom integration.
However there was a lot of complicated code to locally store snapshots that were used as thumbnails for the media platform.
I don't think that was a good idea, it added a lot of complexity to the code and it took a lot of storage drive space on the HomeAssistant device which then needed cleanup services etc. etc.
Bottom line: I would suggest not adding thumbnails unless we can convince reolink to build that into their firmware such that we can simply request a thumbnail for a video from the search command (and preferably also info on the trigger for that video).

@xannor
Copy link
Contributor Author

xannor commented Mar 15, 2023

I agree, I did not like it either, but "live" thumbs on a raspberry at the time were much worse, as FFMPEG was incredibly slow. But good to know, I'll first tool around with an "extension" integration to start with, and see if I can piggyback off the the official. Then when things are stable we can see about PR's to push parts over.

@xannor
Copy link
Contributor Author

xannor commented Mar 15, 2023

I started working on this and I have a couple of questions.

  1. why don't you "expose" the channel names? For mutli-channel devices, i believe, they can be named, and it would be helpful for finding devices. for example I have a DUO 1st gen and I can name both cameras, however when I added the device to HA, I ended up with duplicate named entities because those channel names are not persisted through. I also need this for the media source as I have to "walk" through the integration, i.e. folders for each device, and then for multi-channel devices, folders for each channel. I could use the numbers but using the names provided by the device (and/or the user) would be better.

  2. why don't you "expose" time? The search function works in device time, and I would prefer to use UTC. The camera time is the best way to convert from UTC to a valid time on the camera.

@starkillerOG
Copy link
Owner

@xannor good to hear you have started to work on it!
To answer your questions:

  1. The channel names are exposed by the camera_name property:

    def camera_name(self, channel: int | None) -> Optional[str]:

    The DUO 1st gen might be an special case not handled properly yet, it is a dual lens camera, I do not have one myself and I don't now how it will be handled, I believe it just has two channels and schould therefore also have two channel names, but I have not tested this.

  2. There is a _subscription_time_difference internall property in the library which gives the time diffrence in seconds between the UTC time of the homeassistant device and the CurrentTime on the camera.
    I will investigate what this "CurrentTime' is, I think it is actually also UTC.
    I don't quite get why you would need this, but if you need it, just add it in a PR.

@xannor
Copy link
Contributor Author

xannor commented Mar 16, 2023

  1. didnt see that, good to know.

  2. The reason I wanted the offset is because the times for recordings are in camera time, with no tz info so to make sure they are accurate I need to convert them to UTC for HA. CurrentTime was just my reference to the GetTime call (I couldnt remember the name when I posted.) The only "reliable" way to convert them is to have the full time info, which includes the timezone, since I have to deal with historical datetimes that can cross daylight savings time zones. I can use the private value you hold with that info.

As for the DUO, it reports channels like an NVR, if you need and debug dumps from it I can provide them, and possibly answer some questions about it as that is what I primarily tested and developed against.

@starkillerOG
Copy link
Owner

@xannor did you setup the DUO 1st gen in the new build in HomeAssistant integration?
Did you experiance any issues or does all look good?
Could you share screenshots of the device pages of both channels?
What was the initial naming of the two devices, and did it also make a third device (NVR)?

@starkillerOG
Copy link
Owner

@xannor about the time, if you now the time_diffrence in seconds between the utc.now() on HomeAssistant and the camera internal time you can always calculate the time you need. Just take the datetime from the recording and add/substract the time diffrence in seconds, to get UTC on the HomeAssistant device.

Normally this time diffrence will only be a second or so, but if the camera timezone is set incorrectly lets say 2 hours in front the timediffrence will become 7200 seconds (two hours).

But I think the conversion/correction to UTC timezone schould happen inside the reolink-aio library in the request_vod_files function if that is not yet done.

I have not yet looked at the request_vod_files function, but that needs to be cleaned up and probably some more parsing/processing of the resulting data schould happen in that function.
Probably that function schould return a list of video files available with each item something like a dataclass wich has all the info we need and already processed such that it can easily be used in HomeAssistant.

@starkillerOG
Copy link
Owner

@xannor in this commit I have already cleaned up the request_vod_files method a bit 4fa3a87

But further processing is probably a good idea, convert the dict to a class with date_time objects with all time conversions already taken care of

@starkillerOG
Copy link
Owner

Also cleaned up get_vod_source method a bit in this commit: 80affcd

@xannor
Copy link
Contributor Author

xannor commented Mar 18, 2023

Time:
The main concern/issue is that the camera does not store or present in UTC but localtime w/o tz info. For a non DST timezone this is no issue, however for a TZ that uses DST half the year, historical times would be an hour off. So if you have a device with recordings that cross a time change, without the correct TZ info, they would be off. I have code that I wrote in my libs that would parse the GetTime/Dst info into an actual python tzinfo, and when merged with the Time data would give a datetime with the correct TZ that can be used to convert to/from UTC/localtime for any date. TZ class

DUO:
I installed it when I started working on the media stuff about a week ago, right now into, 2022.3.4. I had no issues with the setup, and it did create entities for both cameras in the device, but as I stated, both sets of entities were created with the same names, as if two separate cameras were mashed together, instead of being prefixed with the name of the channel. Also only 1 device was created, which is ok, because in my code I had created a main device and a sub device for each channel, but it always looked/felt weird to me that HA would list all three devices, and not duplicate missing info into the sub devices, so they always looked incomplete to me.

--Edit: Also, this is a minor thing, but it surprised me. I often reset the DUO, so I have not "initialized it" and your integration requires a password, so the default admin/no password is impossible to use. Not a big issue, but it is something (you could even use that as an initialization approach and require a password to be setup.) Also, I have not tested this, but there was an issue reported in my CI repo about some special characters not working, possibly due to the default url encoding. I dont know if this is something mentioned or tested in your integration, or just something I did wrong in mine.

@xannor
Copy link
Contributor Author

xannor commented Mar 18, 2023

I have found something to add though, for channel capabilities, recDownload and recReplay. These should show if a channel is allowed to use the Download action or if it needs to replay via url (which I think may be less optimal in most cases.) I see you have code to get the vod_source, but not to download the file direct (3.6.6 in the api doc) I know the browser interface does the download call, and I think we can "stream" the download. This may work around the rtmp HD issue as well.

@xannor
Copy link
Contributor Author

xannor commented Mar 19, 2023

I started going down the path of using a Calendar as the "entity" managing the search (it mostly fist concept wise.) and in the process I re-build my tz code from my other integration. If you are curios about and/or want to critique it: DateTime utils

Seems to work so far but it is still very much experimental.

@starkillerOG
Copy link
Owner

I have found something to add though, for channel capabilities, recDownload and recReplay. These should show if a channel is allowed to use the Download action or if it needs to replay via url (which I think may be less optimal in most cases.) I see you have code to get the vod_source, but not to download the file direct (3.6.6 in the api doc) I know the browser interface does the download call, and I think we can "stream" the download. This may work around the rtmp HD issue as well.

If we use the download option, where does the file get downloaded to?
Is it automatically removed from the HomeAssistant device when we stop streaming from HomeAssistant?

@xannor
Copy link
Contributor Author

xannor commented Mar 22, 2023

From the API Docks the download action just provides the MP4 as a data stream over http as apposed to pushing through one of the rtmp/rtsp servers. Since the the files are complete I figure this would be better on the camera.

@starkillerOG
Copy link
Owner

From the API Docks the download action just provides the MP4 as a data stream over http as apposed to pushing through one of the rtmp/rtsp servers. Since the the files are complete I figure this would be better on the camera.

Have you tried this with HomeAssistant?
Is HomeAssistant capable of handeling MP4 data streams?

I am starting to think it would be wise to have a configuration option to specify the playback protocol with options such as MP4, RTSP, RTMP etc.....

@xannor
Copy link
Contributor Author

xannor commented Apr 1, 2023

I have not tried the download command from the API yet, but I have used the replay through RTMP, and I am hoping the download method will solve some of the issues I found.

  1. Last I checked, only RTMP supports playback of recorded, but RTMP cannot send 4k so only sub-streams or less could be transferred.
  2. RTMP playback is every bit as slow to launch as the live stream.
  3. Either the RTMP service, or the HA player, expects the stream to be live, so you lose any ability to seek the stream and do not know its length, only what has buffered so far. Also when the file ends the stream just freezes, nothing indicates and end of stream.

I dont plan on actually downloading full files to the HA install, just streaming the download through the camera stream service. I am hoping this would both load faster and provide the ability to seek the playback, in addition to providing the full quality, and possibly the ability of the user to save the playback somewhere if they choose.

@xannor
Copy link
Contributor Author

xannor commented Apr 12, 2023

OffTopic>
I just updated my test environment to HA 23-4-2 and the DUO does show up a little better there. One thing I would like to see is the default names for the channel based sensors/entries to inherit the channel name of the lens, right now only the cameras inherit the name so I get two motion sensors with the same name for each type of sensor. Also, and this is something I dont know how to fix, even though the duo allows and presents the floodlight on each channel, only 1 switch physically exists so flipping either will turn the light on. This is not an issue except that the other switch does not update when you flip them so it looks odd. No a major issue but something you might get complaints about if someone does not know the difference.
/OffTopic

On the topic front, I am learning to "Decode" the vod filenames, I believe they have the motion sensor data encoded in them, and I can use that in the search to "client side" filter and display the triggers for the file. Unfortunatly I can only get filenames for pet/person/motion/and timer, I dont have a camera in a location that can do vehicle detection, and I dont have anything that supports the other types of detection in the API (face, animal, etc). I think the format has space for those types but I cannot verify.

@xannor
Copy link
Contributor Author

xannor commented Apr 15, 2023

ran into a snag with your library. You code assumes that if a VOD search returns no files but files were requested, it is an error. I think the cameras (at least the duo I am testing with) will also return no files and just a status if there are too many results. While technically correct, it also eats the Status entry so I cannot trap and work around the error (i.e. know which days of the month do contain files and split my request up). If you could either not throw when no file results and pass an empty list, or throw a custom error with the status attached that I can trap, that would help "Work around" this issue.

Update:
I created a separate issue for this #21 and submitted a fix for it as well #22

@starkillerOG
Copy link
Owner

@xannor thanks for figuring out this bug, my NVR always returns a (very long) list of recordings, but if I chose a time-intervall where there are no recordings I would indeed get the error as you posted. I have fixed by simply return a empty list.

Besides I already did some of the parsing in this library using the new VOD_file class. Please have a look at it, and add properties if you need more in a PR.
It is available in the just released reolink_aio v0.5.13

My simple test code:

vod_files = await host.request_vod_files(channel=0, start=datetime.now() - timedelta(days=2), end=datetime.now(), stream="sub")
for file in vod_files[1]:
        print(file)
        print(file.file_name)

If your DUO cam does not return any results if there schould be results because there are too many, please make some additonal code that re-requests the vod_files in chunks and stitches them together afterwords.

@starkillerOG
Copy link
Owner

@xannor

I am learning to "Decode" the vod filenames

Please add this decoding to the new VOD_file class such that we have a simple property (preferably a ENUM) that indicates the type of the recording, we can default to unkown if not available.

@xannor
Copy link
Contributor Author

xannor commented Apr 19, 2023

I looked over your code, and even though you mean well, your simplified UTC code has issues. Mostly, it does not handle daylight savings time, so a camera in a DST zone would be an hour off half the year if the VODs looked up were recorded in before/after a time change. This is because the camera always stores and reports in local time. This is why I went through the trouble of parsing the DST field so I can use the actual TZ info of the camera along with the date/time of the file to determine the actual local time of the file.

For the "flags" because a video can be flagged as multiple "triggers" I am looking into if I should use a intflag or an array of enums, a VOD could have multiple, even potentially none.

defaulting to an empty file list is fine, though I think there is a meaning difference in the API between no files and a missing files. I have noticed that when my search range exceeds the camera history, it would just report the statuses (i.e. as a hint of how far back you can search) even when the range would include files.

@xannor
Copy link
Contributor Author

xannor commented Apr 19, 2023

Some feedback on your VOD_file class, What is playback time? I have not seen that field on my camera, I assume it is an NVR thing (I dont have one of those) Pulling that field, when it is not present would error. Also, why are you taking a dict[str,any] instead of using your SearchFile typeddict?

@xannor
Copy link
Contributor Author

xannor commented Apr 19, 2023

I also did some basic work to continue with the files improvements. Its not ready to submit as I am still working on the filename decoding (and I had to do some other cleanup to handle your recent changes.) hopefully I can have something to push this weekend. I did get the calendar integration basically working with the changes in this fork

@starkillerOG
Copy link
Owner

I looked over your code, and even though you mean well, your simplified UTC code has issues. Mostly, it does not handle daylight savings time, so a camera in a DST zone would be an hour off half the year if the VODs looked up were recorded in before/after a time change. This is because the camera always stores and reports in local time. This is why I went through the trouble of parsing the DST field so I can use the actual TZ info of the camera along with the date/time of the file to determine the actual local time of the file.

I did check for daylight savings time, and it apear to work, the cam_hour_offset = round(self.time_offset / 3600) schould take care of that, but maybe I am wrong.
Although I do image indeed around the time change things would go wrong. So when the DST has just changed and I am looking at recordings that have been recordered before the DST. Does your PR #23 take care of this correctly (my implementation does not)?
However for my NVR I did notice that the reported start time of a recording from the Search result changes when changing the DST time setting, so the Search result is not static but also changes the past files when a DST time change happens, so I was guessing my implementation would actually work just fine.

For the "flags" because a video can be flagged as multiple "triggers" I am looking into if I should use a intflag or an array of enums, a VOD could have multiple, even potentially none.

Ah right, did not consider that.
Maybe then just add a bunch of boolean properties that indicate if a recording has motion/people/vehicle etc event

defaulting to an empty file list is fine, though I think there is a meaning difference in the API between no files and a missing files. I have noticed that when my search range exceeds the camera history, it would just report the statuses (i.e. as a hint of how far back you can search) even when the range would include files.

I never got back a empty file list from my NVR, did you?

@starkillerOG
Copy link
Owner

starkillerOG commented Apr 20, 2023

Some feedback on your VOD_file class, What is playback time? I have not seen that field on my camera, I assume it is an NVR thing (I dont have one of those)

Actually I am not entirely sure yet, it is another timestamp, I think a motion recording (short recording 1 min or so) will have the playback time refering to the start of the bigger 1 hour recording file of which it is part, or something simular, but have not figured out what it actually means. It was just in the return data from the NVR.

Pulling that field, when it is not present would error.

We schould default it to None when it is not present, can be done in a followup PR.

Also, why are you taking a dict[str,any] instead of using your SearchFile typeddict?

Yea that schould probably indeed be the SearchFile typing, but I just wanted to provide some basics to you of what I had in mind. I did not check if the SearchFile typing is consistant with the NVR response.

@xannor
Copy link
Contributor Author

xannor commented Apr 20, 2023

I did check for daylight savings time, and it apear to work, the cam_hour_offset = round(self.time_offset / 3600) schould take care of that, but maybe I am wrong. Although I do image indeed around the time change things would go wrong. So when the DST has just changed and I am looking at recordings that have been recordered before the DST. Does your PR #23 take care of this correctly (my implementation does not)? However for my NVR I did notice that the reported start time of a recording from the Search result changes when changing the DST time setting, so the Search result is not static but also changes the past files when a DST time change happens, so I was guessing my implementation would actually work just fine.

Be carefull relying on "it works on the NVR" it is better hardware than the cameras (and tends to have more bells an whistles) so it may do things the cameras cant. I.E. store in UTC and adjust to local (I dont own an NVR so I dont know what it is fully capable of, I can only guess/assume). The cameras I have tested with only seem to store in local time so changing the time or DST enabled does not seem to affect existing recordings, only new ones. The code I provided just implements their DST rules as a python timezone, with that you can convert the camera localtime to any time.

For the "flags" because a video can be flagged as multiple "triggers" I am looking into if I should use a intflag or an array of enums, a VOD could have multiple, even potentially none.

Ah right, did not consider that. Maybe then just add a bunch of boolean properties that indicate if a recording has motion/people/vehicle etc event

I would prefer to at least use enums in a list as I dont have enough sources to test and determine what all the flags values could be, and there could be more in the future. So far I can verify Timer Motion Pet and Person. I have a couple of cameras that support vehicle, but are not in a location that has any to detect. I know in the API there is also animal (maybe the Trax does this) and face. The only issue with intflags is, depending in the python version. 0 could mean none or all, and I think it may be possible to get a recording with no triggers, not sure how but I have gotten ones with pet and person but no motion.

defaulting to an empty file list is fine, though I think there is a meaning difference in the API between no files and a missing files. I have noticed that when my search range exceeds the camera history, it would just report the statuses (i.e. as a hint of how far back you can search) even when the range would include files.

I never got back a empty file list from my NVR, did you?

I have seen it in the past but that may have been older firmware. I will have to test and see what I get for a response in a range with no recordings, but within the status window. Not that it matters, as long as I get the status I can see if there would have been matches.

@xannor
Copy link
Contributor Author

xannor commented Apr 20, 2023

Some feedback on your VOD_file class, What is playback time? I have not seen that field on my camera, I assume it is an NVR thing (I dont have one of those)

Actually I am not entirely sure yet, it is another timestamp, I think a motion recording (short recording 1 min or so) will have the playback time refering to the start of the bigger 1 hour recording file of which it is part, or something simular, but have not figured out what it actually means. It was just in the return data from the NVR.

Pulling that field, when it is not present would error.

We schould default it to None when it is not present, can be done in a followup PR.

Also, why are you taking a dict[str,any] instead of using your SearchFile typeddict?

Yea that schould probably indeed be the SearchFile typing, but I just wanted to provide some basics to you of what I had in mind. I did not check if the SearchFile typing is consistant with the NVR response.

On a side note, what is the target of this lib ,10 or 11? if 11 I would like to use StrEnums where possible instead of literal strings, if 10 is a possible target, we can either go with Enums and "fake" an StrEnum or use a backport lib. I kinda wish the one in HA was a separate lib instead of module code, then we could use that because it would already be present. I wasnt sure what your thoughts are on this so I haven used any yet.

@starkillerOG
Copy link
Owner

Currently this lib supports python 3.9, 3.10 and 3.11. I think having the last 3 python versions supported is a good thing, once 3.12 is out, I will probably drop 3.9.

There are also pylint, flake8 and mypy tests beeing run for all 3 python versions.

@xannor
Copy link
Contributor Author

xannor commented May 10, 2023

Thanks for the info, and getting the first part merged. I need some help with deciphering the filenames. The best I can "detect" with the cameras I have is the following:
image
I cannot test vehicles because I dont have a camera in a position to accurately detect them. I also know, from the API specs, that there is also face and animal detection types, but I dont have anything that supports that.

I need a file list that includes the missing types detected. The way I have been doing this is using the reolink client to narrow down a start time for a detection type and then looking in the web client download list for the file listed for that time, downloading and verifying that it is correct, then note what the client says its detects and what the download filename is.

If I could get a few of these for the mising types, vehicle, face, and animal. then I can complete the "trigger" decoding, until they add more types.

@xannor
Copy link
Contributor Author

xannor commented Nov 23, 2023

Working on the full dl part now. will possibly have a test for you to try (to make sure NVRs dont get tripped up by it) soon.

my thoughts on the month to month is to take advantage of the status return, and encode the next/current month into the identity (or use some sort of session/temp, I have what question in the HA discord). This way in a given month page we should know if the prior month we came from so we only need to do a status for the beginning or end of the month we want. This would also work since the starting month is current and only the prior month would need to be included (which we can do easily.)

@starkillerOG
Copy link
Owner

Yea I think we can just make 2 extra BrowseMediaSource items, one for the next and one for the previous month. (make them conditionally if we are already at the current month and if the previous month actually has recordings).
In the identifier of those two items we can just encode to wich new month we schould go.
I don't think it should be too difficult to implement.

@xannor
Copy link
Contributor Author

xannor commented Nov 23, 2023

It would also be good to keep track of which direction we came from, so we only have to query in one direction on the camera, which is why I had the question about session/temp (in discord), otherwise could encode it in the identifier, but wanted to avoid polluting it if possible.

@xannor
Copy link
Contributor Author

xannor commented Nov 23, 2023

Got the basics for DL support here https://github.com/xannor/ha-core/tree/reolink_vod_dl however, having an issue where the download is aborted about 30 seconds in, dont know if it is because of aio timeouts or something in the api side. Just get a connection closed notice. Give it a test and see if it causes any problems with NVRs though.

@starkillerOG
Copy link
Owner

@xannor sorry for taking so long, I just tested your reolink_vod_dl branch with my NVR but it does not work.
host.api.api_version("recDownload", channel) is also 1 for all channels of the NVR, (because it does support downloading, but in a diffrent way).
So the NVR now also attempts to do a download and fails.

So we will need to check using host.api.is_nvr and make sure the NVR is still using the streaming instead of downloading.
Later we could also try and see if we can get the downloading working on the NVR (but I think we would need a sequence of two commands to get that working).

@xannor
Copy link
Contributor Author

xannor commented Nov 26, 2023

Thats what I was not sure of, and why I needed you to test. does the NVR fail with a command error or does the D/L start but fail after some point? The API's should match the "abilities" and so I want to make sure we go down the right path and not just make assumptions.

@starkillerOG
Copy link
Owner

This commit implemented download support for the NVR: 27e5124
It is tested and works (with some small adjustments to your branch).

However I would not recommend it for an NVR since it takes a really long time to start a playback (NvrDownload takes something like 20 seconds before completing).
Moreover it is easy to upset the NVR, when requesting the same download for instance to fast after eachother or opening another download while the first one was not done closing yet (clicking fast in HA) causes the NVR to return a "busy" error. Then you have to wait something like an hour or reboot the NVR to get it out of the "busy" state....
And the Ext stream is not supported for NVRs.

I will make a PR for your branch to suggest the nessesary changes.

Maybe we can disable download for NVRs at first and in a seperate PR once this is merged make a new config flow option to let the user chose between streaming/downloading for playback (the NVR will have streaming as default and other cams downloading as default).
Or we could make options in the browsing navigation (like at the resolution page, make Streaming/Downloading options...)

@starkillerOG
Copy link
Owner

See my PR to your branch here: xannor/ha-core#1

@xannor
Copy link
Contributor Author

xannor commented Nov 29, 2023

I think for the time being, just not doing the download on the NVR is the better approach, as there is little advantage in that instance.

I still feel that an NVR is fundementally the same approach as here and it would be better to manage/use that interface than try to use the limited API and have HA do it.

I mostly want this for those who do not have/want an NVR and want to have some of the functionality in concept.

Also, I am not comfortable doing changes that affect/use the NVR as I do not have one and could not test any issues related to it.

@xannor
Copy link
Contributor Author

xannor commented Nov 29, 2023

I looked at your PR. the live ability should be 1 if main/sub/ext is supported and 2 if only main/sub according to the API docs. you said the NVR does not support ext, does it report a 1 for that value, because I though I was explicitly checking for 1 in hasExt.

@starkillerOG
Copy link
Owner

starkillerOG commented Nov 29, 2023

Yes the NVR was reporting 1 for the "live" ability, but it only has main/sub playback, not ext.
It does support live realtime streaming of the ext stream (and main/sub) on RTMP or FLV, (not RTSP).
The download did not work when Ext was used as stream.

@xannor
Copy link
Contributor Author

xannor commented Nov 29, 2023

its nice to see the NVR does not follow the documented API correctly.. :) I'll add the NVR check to that as well.

Ahh, I see your edit, so it does support ext for live but not playback. Given my tests where ext and sub seem the same on HD and 4MP cameras, maybe I should just skip ext for recordings? They do seems smaller so I dont know if it is a client side resize that makes them appear the same to me.

@xannor
Copy link
Contributor Author

xannor commented Nov 29, 2023

I did some small edits to disable NVR DL and fully disabled ext playback, for now. I still need to investigate the DL issues I have seen, but could you check that the NVR is working as it should now?

@xannor
Copy link
Contributor Author

xannor commented Nov 29, 2023

Ok, interesting results. I changed the player to pass the url to the camera though (since the command can be called via get like caps can.) for the non-hd it played right away and could play videos longer than 30sec, works wonderfully. For HD because the h265 codec, firefox refused to play, but could download if I open link in new tab, and MS edge would only play audio. So it looks like the HD cams will be an issue with playback. The downloaded file plays fine though. I also could not open the file via VLC (with the link) but that might be because the cams only allow 1 request to download they will deny and throw fits to multiple requests, which VLC might be doing.

I did determine the issues I was having in passing the stream through HA are due to the connection timeouts, but I am not sure I can control them entirely. The alternative is to pass the url through (with token) but that both potentially exposes security info, and would not work with clients that do not have direct access to the camera themselves. Also if the camera is setup for the default https cert, the browser will reject the connection anyway.

I am not sure how to be proceed from this point,

@starkillerOG
Copy link
Owner

@xannor I left some comments here: xannor/ha-core#1

Yes h265 encoding can be a problem to play in some browsers, that is also why the RTSP stream from Reolink does not yet support h265 encoding I think.

I would just do only sub stream for cameras that have h265 encoding, just like what is done for the NVR.

I changed the player to pass the url to the camera though

I don't quite understand this...

The alternative is to pass the url through (with token) but that both potentially exposes security info, and would not work with clients that do not have direct access to the camera themselves.

I also don't understand this, could you give an example of the URL you are talking about? (you can replace token/password with xxxx)

@xannor
Copy link
Contributor Author

xannor commented Dec 3, 2023

Yes h265 encoding can be a problem to play in some browsers, that is also why the RTSP stream from Reolink does not yet support h265 encoding I think.

I beleive the RTSP does do h265, as that is the only way to pull the full stream, RTMP does not support h265.

I would just do only sub stream for cameras that have h265 encoding, just like what is done for the NVR.

Yeah, might have to but I dont like it. Would be nice if HA had a download component, for downloading from the HA, and not just downloading to it.

I changed the player to pass the url to the camera though

I don't quite understand this...

Just like with the screen capture API command, the download command can be passed to the camera as a GET style url request. I manually built that in the code to test, and it does work, except for the afformentioned h265 problem.

The alternative is to pass the url through (with token) but that both potentially exposes security info, and would not work with clients that do not have direct access to the camera themselves.

I also don't understand this, could you give an example of the URL you are talking about? (you can replace token/password with xxxx)

When passing the download command as a url, you have to supply the token parameter as well. If we send this url to the client, it means internal security information (normally the token is not exposed anywhere) would be sent over the web socket. Even though this should be safe, I dont like the thought of doing that, if it can be avoided.

The reason I tried this approach is because all view requests in HA, by default use the default timeout of 30 seconds for the total life of the session connection. Since proxing the file through HA means two connections, one on the camera side the integration can control, and one on the HA side that core controlls, if the file takes longer than 30 seconds to transfer, it gets killed by the timeout. Most players will only buffer about 10-15 seconds and pause thedownload, so almost all transfer of files larger than a 15 second video will fail.

Passing the url through works around this because now HA is not involved with the transfer, however, this will not work in situations where the client must access content though HA (such as remote connections) as the url would not be properly resolvable to the client.

In addition if the connection is going over SSL, and a self-signed cert is used. then the player will not work anyways because the browser would reject the cert, proxying through HA also worked around this issue.

Also, passing the url like that to potentially a remote client means that url would now be re-transmitted over the internet when the client attempts to connect.

Both of these situations make me not want to try to hand the url off to the client as it would lead to a bad experience that a regular user may not realize is not an error in the integration. Thus needless bug tickets.

@xannor
Copy link
Contributor Author

xannor commented Dec 6, 2023

From the absence in the write-up, I'm guessing the media support did not make the cut for this release? Updating right now so I will know for sure in a few minutes.

@starkillerOG
Copy link
Owner

@xannor I just updated and it is included, it is just not mentioned in the release notes.

@xannor
Copy link
Contributor Author

xannor commented Dec 6, 2023

Yep, i see it, surprised there was no mention in the write-up.

Hah, the snowflake camera work with it, but boy is it ugly, just a wall of icons and times. Not the integrations fault, the camera only reports motion events and it sure detects a lot of leaves moving during the day.

I wish there was a way to localize the info better, for instance, show times in 12hr instead of 24hr as that is more "natural" for over here. What you can and cannot localize has been somewhat of a mystery to me.

@starkillerOG
Copy link
Owner

We could localize if you want.
You can probably get the global HA settings for time format configured.
Then we could use that to dynamically change the title style to 24 vs 12 PM...

@xannor
Copy link
Contributor Author

xannor commented Dec 6, 2023

is there a way to see the user preferences? Going that route I would prefer to use user settings instead of system, if possible.

I have tried to dig through other integrations to see how they handle that sort of thing, but mostly when localization has been involved, they only talk about states and other things done through the lang files, not regular output.

Also, is there a way to use the lang data as well, would be good to use localized text for the AI states from the lang for the user than forcing english, for the VOD info.

@xannor
Copy link
Contributor Author

xannor commented Dec 13, 2023

I was re-reading the API and I see a "new" command called playback that I didnt see before. It is structured exactly like download but does not list the same limitations. I am going to test with it and expand the vod_source function to handle generate one of the three possible urls.

Also in vod source I see you check for rtmp and enable it, this should be the responsibility of the integration to en/force not as a side-effect of the library (generally side-effects are a bad idea). I am going to remove this from my changes.

Also, also, I am going to remove the download support from the get_chunk as I think using vod_source and letting the integration deal with the url is better than passing back an abusable stream (solves some of the hacks to get the stream out of get_chunk properly.)

@xannor
Copy link
Contributor Author

xannor commented Dec 13, 2023

Ok got a little farther, and feel a little better about it. I have a new PR to this lib for the changes I need (and I updated my branch of core). If you could test what I have with your NVR to make sure I didnt break anything, this would be a good start for the second round of media support.

@starkillerOG
Copy link
Owner

I will try looking at it soon, am a bit swamped with work at the moment tho...

Seems like a nice way to go forward.

@starkillerOG
Copy link
Owner

@xannor I just merged the PR (in a seperate comment), I will release a new reolink_aio version shortly.

@xannor
Copy link
Contributor Author

xannor commented Dec 30, 2023

Ahh, I see where you thought things didnt work. I was not enforcing the RTMP/DL limit in API but in the component, I figure the API should be closer to "dumb" and the calling code should enforce any device specific rules, except where the API docs state such a rule, or a work around is necessary.

@starkillerOG
Copy link
Owner

I like to put as much logic in the library and make the HomeAssistant code as "dumb" as possible.
Because changes in the library are easy and a HA version bump will be done within a day.
However changing logic in HA can take weeks because of waiting on reviews.....

@xannor
Copy link
Contributor Author

xannor commented Dec 30, 2023

Yeah, I see where your coming from now. Its just an old habit of mine, to write as close to the API and UI as possible and bridge the difference.

@starkillerOG
Copy link
Owner

@xannor version bump PR in HA is here: home-assistant/core#106747
I needed to add a small bit of logic to restore the default behaviour of RTMP for seperate cams and FLV for NVRs.

@starkillerOG
Copy link
Owner

@xannor could you check this issue: home-assistant/core#110939
Is there something wrong with the playback URL?
I don't have standalone cams with SD cards, so can not easily check myself.

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

No branches or pull requests

5 participants