Fix reolink media playing on webkit browser#169184
Fix reolink media playing on webkit browser#169184mrtncode wants to merge 4 commits intohome-assistant:devfrom
Conversation
|
Hey there @starkillerOG, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
@starkillerOG Can you take a look? It works great on all apple devices (tested on iphone and mac) but it need much time to load (10-15 seconds). Any idea how to imporove this? And the code is not ready/ good. Its rather a functional prototype 😅 |
| _mime_type, reolink_url, total_length_hint = await host.api.get_vod_source( | ||
| ch, | ||
| filename_decoded, | ||
| stream_res, | ||
| VodRequestType(vod_type), | ||
| include_total_length=True, | ||
| ) |
There was a problem hiding this comment.
This changes the expected signature/return shape of host.api.get_vod_source (adds include_total_length kwarg and a third return value). If the host API and/or underlying reolink_aio dependency isn’t updated in the same PR, this will raise at runtime (unexpected kwarg and/or tuple unpacking error). Consider keeping backward compatibility by supporting both 2-tuple and 3-tuple returns (and only passing the kwarg when supported), or update the host API wrapper and pinned dependency together in this PR.
|
|
||
| requires_auth = True | ||
| url = "/api/reolink/video/{config_entry_id}/{channel}/{stream_res}/{vod_type}/{filename}" | ||
| url = "/api/reolink/video/{config_entry_id}/{channel}/{stream_res}/{vod_type}/{filename}.mp4" |
There was a problem hiding this comment.
The route is changed to require a literal .mp4 suffix. That can be a breaking change for any existing callers (bookmarked URLs, external clients, or older frontend code) that still use the previous endpoint without the suffix, while the PR is marked as a non-breaking bugfix. If backward compatibility is needed, add an additional route for the old path (or accept both patterns) and keep behavior consistent between them.
| ssl_cipher=SSLCipherList.INSECURE, | ||
| ) | ||
| self._vod_type: str | None = None | ||
| self._size_cache: dict[str, int] = {} |
There was a problem hiding this comment.
The _size_cache is unbounded and keyed by reolink_url. If that URL contains per-request/session tokens (common for camera VOD URLs), this can grow without limit over time and also yield low cache hit rates. Consider bounding the cache (LRU/TTL) and/or using a stable cache key (e.g., config entry + channel + stream + filename) and clearing entries when sessions are expired/rotated.
| if (cached := self._size_cache.get(reolink_url)) is not None: | ||
| return cached | ||
|
|
||
| if total_length_hint is not None: | ||
| self._size_cache[reolink_url] = total_length_hint | ||
| return total_length_hint |
There was a problem hiding this comment.
The _size_cache is unbounded and keyed by reolink_url. If that URL contains per-request/session tokens (common for camera VOD URLs), this can grow without limit over time and also yield low cache hit rates. Consider bounding the cache (LRU/TTL) and/or using a stable cache key (e.g., config entry + channel + stream + filename) and clearing entries when sessions are expired/rotated.
| self._size_cache[reolink_url] = length | ||
| return length |
There was a problem hiding this comment.
The _size_cache is unbounded and keyed by reolink_url. If that URL contains per-request/session tokens (common for camera VOD URLs), this can grow without limit over time and also yield low cache hit rates. Consider bounding the cache (LRU/TTL) and/or using a stable cache key (e.g., config entry + channel + stream + filename) and clearing entries when sessions are expired/rotated.
| async def _async_resolve_total_length( | ||
| self, | ||
| host: Any, | ||
| channel: int, | ||
| stream_res: str, | ||
| filename_decoded: str, | ||
| reolink_url: str, | ||
| reolink_response: ClientResponse, | ||
| total_length_hint: int | None = None, | ||
| ) -> int | None: |
There was a problem hiding this comment.
_async_resolve_total_length currently accepts host, channel, stream_res, and filename_decoded but (in this diff) doesn’t use them. In Home Assistant’s Ruff configuration, unused arguments may be flagged, and it also makes the helper harder to understand. Either remove these parameters, prefix them with _ to indicate intentional unused args, or use them to build a stable cache key (which also helps avoid caching by a potentially tokenized URL).
| async def _async_stream_passthrough( | ||
| self, | ||
| request: web.Request, | ||
| host: Any, | ||
| reolink_response: ClientResponse, | ||
| ) -> web.StreamResponse: | ||
| """Stream the upstream response without any range synthesis.""" | ||
| response_headers = dict(reolink_response.headers) | ||
| response_headers.pop("Content-Disposition", None) | ||
| response_headers.pop("content-disposition", None) | ||
| self._normalize_content_type(response_headers, reolink_response.content_type) |
There was a problem hiding this comment.
Hop-by-hop headers are stripped only in the WebKit branch (_HOP_BY_HOP_RESPONSE_HEADERS), but passthrough streaming can also propagate hop-by-hop headers like Connection or Transfer-Encoding directly from upstream, which is generally incorrect for a proxy response. Consider applying the same hop-by-hop header removal in _async_stream_passthrough as well.
| if not self._is_webkit_client(request): | ||
| return await self._async_stream_passthrough(request, host, reolink_response) | ||
|
|
||
| total_length = await self._async_resolve_total_length( | ||
| host, | ||
| ch, | ||
| stream_res, | ||
| filename_decoded, | ||
| reolink_url, | ||
| reolink_response, | ||
| total_length_hint, | ||
| ) | ||
| force_range, content_range_total, error_response = self._plan_range_handling( | ||
| request, reolink_response, total_length | ||
| ) |
There was a problem hiding this comment.
This adds substantial new behavior for WebKit clients (user-agent detection, range synthesis, 416 handling, and optional probing for total length) but the PR description notes tests are not updated yet. Add tests covering at least: (1) Safari/WebKit Range: bytes=0-1 probe resulting in 206 with correct Content-Range/Content-Length, (2) invalid range returning 416 with Content-Range: bytes */<total>, and (3) non-WebKit clients continuing to passthrough without synthesized range behavior.
Breaking change
Proposed change
This PR fixes a bug where the reolink videos (from media_source) cannot be played on Webkit/ Apple Devices. This PR fixes that by using the ios/ webkit requirements for streaming the videos (especially request headers).
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests:
PR is still WIP. The tests has not been updated yet
ToDos: