feat: add support for the new download mechanism used in Unifi OS 4 #38
Conversation
Copy pull request changes I proposed to pyunifiprotect Signed-off-by: cyberpower678 <[email protected]>
WalkthroughThe recent updates to Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant API as uiprotect.api
participant Camera as Camera
User ->> API: prepare_camera_video(start, end, ...)
API ->> Camera: Prepare video (start, end, ...)
Camera -->> API: Prepared video info
User ->> API: download_camera_video(prepared_video_info, ...)
API ->> Camera: Download video chunks
Camera -->> API: Video chunks
API ->> User: MP4 Video
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
src/uiprotect/api.py (3)
Line range hint
1454-1726: Review the usage ofOptionalandUniontypes.The static analysis tool has identified multiple instances where
OptionalandUniontypes are used but not defined. This will cause runtime errors. To resolve this, you should import these types from thetypingmodule at the beginning of your file.+ from typing import Optional, UnionTools
Ruff
1458-1458: Undefined name
Optional(F821)
1459-1459: Undefined name
Optional(F821)
1460-1460: Undefined name
Optional(F821)
1462-1462: Undefined name
Optional(F821)
1525-1525: Undefined name
Optional(F821)
1526-1526: Undefined name
Optional(F821)
1527-1527: Undefined name
Optional(F821)
1527-1527: Undefined name
Union(F821)
1588-1588: Undefined name
Optional(F821)
1589-1589: Undefined name
Optional(F821)
1590-1590: Undefined name
Optional(F821)
1592-1592: Undefined name
Optional(F821)
1593-1593: Undefined name
Optional(F821)
1454-1462: Ensure proper error handling indownload_camera_video.The method
download_camera_videolacks explicit error handling for network or IO operations. Consider adding try-except blocks around the network calls and file operations to handle potential exceptions and provide a more robust error handling mechanism.Tools
Ruff
1458-1458: Undefined name
Optional(F821)
1459-1459: Undefined name
Optional(F821)
1460-1460: Undefined name
Optional(F821)
1462-1462: Undefined name
Optional(F821)
1669-1725: Deprecation ofget_camera_videoneeds clear documentation.The method
get_camera_videois deprecated, but the documentation could be clearer about what users should do if they are using versions of Unifi Protect older than 4. Consider enhancing the documentation to guide users more clearly.Tools
Ruff
1676-1676: Undefined name
Optional(F821)
1677-1677: Undefined name
Optional(F821)
1678-1678: Undefined name
Optional(F821)
1680-1680: Undefined name
Optional(F821)
1681-1681: Undefined name
Optional(F821)
1682-1682: Undefined name
Optional(F821)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/uiprotect/api.py (4 hunks)
Additional context used
Ruff
src/uiprotect/api.py
1458-1458: Undefined name
Optional(F821)
1459-1459: Undefined name
Optional(F821)
1460-1460: Undefined name
Optional(F821)
1462-1462: Undefined name
Optional(F821)
1525-1525: Undefined name
Optional(F821)
1526-1526: Undefined name
Optional(F821)
1527-1527: Undefined name
Optional(F821)
1527-1527: Undefined name
Union(F821)
1588-1588: Undefined name
Optional(F821)
1589-1589: Undefined name
Optional(F821)
1590-1590: Undefined name
Optional(F821)
1592-1592: Undefined name
Optional(F821)
1593-1593: Undefined name
Optional(F821)
1676-1676: Undefined name
Optional(F821)
1677-1677: Undefined name
Optional(F821)
1678-1678: Undefined name
Optional(F821)
1680-1680: Undefined name
Optional(F821)
1681-1681: Undefined name
Optional(F821)
1682-1682: Undefined name
Optional(F821)
|
Thanks for the PR, I'll add some comments to get the review started shortly |
|
Let me conform it first to the way you have the typing handled. :-) |
src/uiprotect/api.py
Outdated
| self, | ||
| camera_id: str, | ||
| filename: str, | ||
| output_file: Optional[Path] = None, |
There was a problem hiding this comment.
Please use pep 0563 style
pyupgrade can likely convert these for you
There was a problem hiding this comment.
| if isinstance(prepare_response, dict): | ||
| download_filename = prepare_response["fileName"] | ||
| else: | ||
| raise Exception |
There was a problem hiding this comment.
Please avoid using a broad exception raise in a try block to control flow
| progress_callback=progress_callback, | ||
| chunk_size=chunk_size, | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
Please avoid a broad exception catch and only catch exceptions that would confirm the need to fallback to the other method as it can unexpectedly hide bugs
There was a problem hiding this comment.
This was quite intentional, as it's supposed to fall back on the previous video/export on any kind of error, including using it on a pre-4 Protect installation whose responses I can't predict since I have no platform to test it on. The get_camera_video() needs to be deprecated in favor of either using the export_camera_video method, which is functionally identical, or the new prepare/download_camera_video methods used in Protect 4.
There was a problem hiding this comment.
It's better for it to fail than to have undefined behavior since if it falls back without warning the problem will never get fixed, and when the deprecated function is removed we will find out the hard way when it breaks which will be harder to fix.
Please narrow the exception catch to only exceptions that are expected that should trigger the fallback.
https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/broad-exception-caught.html
There was a problem hiding this comment.
This also needs a test to simulate a failure so we know under which conditions it should fallback to the old api
There was a problem hiding this comment.
It should also log a message when it falls back so we know it's happening so we have a chance to fix whatever is causing the failure
| @@ -1451,25 +1451,152 @@ async def _stream_response( | |||
| if progress_callback is not None: | |||
| await progress_callback(step, current, total) | |||
|
|
|||
| async def get_camera_video( | |||
| async def download_camera_video( | |||
There was a problem hiding this comment.
Please add some tests for the new methods
Signed-off-by: cyberpower678 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/uiprotect/api.py (6 hunks)
Additional context used
Ruff
src/uiprotect/api.py
1680-1680: SyntaxError: Parameter without a default cannot follow a parameter with a default (E999)
src/uiprotect/api.py
Outdated
| async def prepare_camera_video( | ||
| self, | ||
| camera_id: str, | ||
| start: datetime, | ||
| end: datetime, | ||
| channel_index: int = 0, | ||
| validate_channel_id: bool = True, | ||
| fps: int | None = None, | ||
| filename: str | None = None, | ||
| ) -> list[Any] | dict[str, Any] | None: | ||
| """ | ||
| Prepares MP4 video from a given camera at a specific time. | ||
|
|
||
| This is the newer implementation of retrieving video from Unifi Protect. | ||
| When using this method, it should be followed up with download_camera_video(). | ||
|
|
||
| Start/End of video export are approximate. It may be +/- a few seconds. | ||
|
|
||
| Providing the `fps` parameter creates a "timelapse" export wtih the given FPS | ||
| value. Protect app gives the options for 60x (fps=4), 120x (fps=8), 300x | ||
| (fps=20), and 600x (fps=40). | ||
|
|
||
| You will receive a filename and an expiry time in seconds. | ||
| """ | ||
| if validate_channel_id and self._bootstrap is not None: | ||
| try: | ||
| camera = self._bootstrap.cameras[camera_id] | ||
| camera.channels[channel_index] | ||
| except IndexError as e: | ||
| raise BadRequest from e | ||
|
|
||
| params = { | ||
| "camera": camera_id, | ||
| "start": to_js_time(start), | ||
| "end": to_js_time(end), | ||
| } | ||
|
|
||
| if channel_index == 3: | ||
| params.update({"lens": 2}) | ||
| else: | ||
| params.update({"channel": channel_index}) | ||
|
|
||
| if fps is not None and fps > 0: | ||
| params["fps"] = fps | ||
| params["type"] = "timelapse" | ||
| else: | ||
| params["type"] = "rotating" | ||
|
|
||
| if not filename: | ||
| start_str = start.strftime("%m-%d-%Y, %H.%M.%S %Z") | ||
| end_str = end.strftime("%m-%d-%Y, %H.%M.%S %Z") | ||
| filename = f"{camera_id} {start_str} - {end_str}.mp4" | ||
|
|
||
| params["filename"] = filename | ||
|
|
||
| path = "video/prepare" | ||
|
|
||
| return await self.api_request( | ||
| path, | ||
| params=params, | ||
| raise_exception=True, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Ensure proper error handling in prepare_camera_video.
- return await self.api_request(
+ try:
+ return await self.api_request(
- )
+ )
+ except Exception as e:
+ _LOGGER.error(f"Failed to prepare video: {str(e)}")
+ raiseAdding exception handling to prepare_camera_video will help in identifying and logging issues that may occur during the API request, improving the method's reliability.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def prepare_camera_video( | |
| self, | |
| camera_id: str, | |
| start: datetime, | |
| end: datetime, | |
| channel_index: int = 0, | |
| validate_channel_id: bool = True, | |
| fps: int | None = None, | |
| filename: str | None = None, | |
| ) -> list[Any] | dict[str, Any] | None: | |
| """ | |
| Prepares MP4 video from a given camera at a specific time. | |
| This is the newer implementation of retrieving video from Unifi Protect. | |
| When using this method, it should be followed up with download_camera_video(). | |
| Start/End of video export are approximate. It may be +/- a few seconds. | |
| Providing the `fps` parameter creates a "timelapse" export wtih the given FPS | |
| value. Protect app gives the options for 60x (fps=4), 120x (fps=8), 300x | |
| (fps=20), and 600x (fps=40). | |
| You will receive a filename and an expiry time in seconds. | |
| """ | |
| if validate_channel_id and self._bootstrap is not None: | |
| try: | |
| camera = self._bootstrap.cameras[camera_id] | |
| camera.channels[channel_index] | |
| except IndexError as e: | |
| raise BadRequest from e | |
| params = { | |
| "camera": camera_id, | |
| "start": to_js_time(start), | |
| "end": to_js_time(end), | |
| } | |
| if channel_index == 3: | |
| params.update({"lens": 2}) | |
| else: | |
| params.update({"channel": channel_index}) | |
| if fps is not None and fps > 0: | |
| params["fps"] = fps | |
| params["type"] = "timelapse" | |
| else: | |
| params["type"] = "rotating" | |
| if not filename: | |
| start_str = start.strftime("%m-%d-%Y, %H.%M.%S %Z") | |
| end_str = end.strftime("%m-%d-%Y, %H.%M.%S %Z") | |
| filename = f"{camera_id} {start_str} - {end_str}.mp4" | |
| params["filename"] = filename | |
| path = "video/prepare" | |
| return await self.api_request( | |
| path, | |
| params=params, | |
| raise_exception=True, | |
| ) | |
| try: | |
| return await self.api_request( | |
| path, | |
| params=params, | |
| raise_exception=True, | |
| ) | |
| except Exception as e: | |
| _LOGGER.error(f"Failed to prepare video: {str(e)}") | |
| raise |
src/uiprotect/api.py
Outdated
| iterator_callback: IteratorCallback | None = None, | ||
| progress_callback: ProgressCallback | None = None, | ||
| chunk_size: int = 65536, | ||
| fps: int | None, |
There was a problem hiding this comment.
Correct the syntax error in the method signature.
- fps: int | None,
+ fps: int | None = None,Parameters without default values should not follow parameters with default values. This change corrects the method signature to comply with Python's syntax rules.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fps: int | None, | |
| fps: int | None = None, |
Tools
Ruff
1680-1680: SyntaxError: Parameter without a default cannot follow a parameter with a default (E999)
|
When I get back home I'll adjust the settings so you don't have to wait for the ci to rerun |
Signed-off-by: cyberpower678 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
src/uiprotect/api.py (1)
Line range hint
1581-1630: Consider simplifying the handling of optional parameters.The method
export_camera_videohandles optional parametersfpsandchannel_indexby checking their values and updating the parameters dictionary conditionally. This could be simplified by setting default values in the parameters dictionary and updating them only if necessary.- if channel_index == 3: - params.update({"lens": 2}) - else: - params.update({"channel": channel_index}) - if fps is not None and fps > 0: - params["fps"] = fps - params["type"] = "timelapse" + params.update({"channel": channel_index if channel_index != 3 else "lens": 2}) + if fps > 0: + params.update({"fps": fps, "type": "timelapse"})This refactoring uses a more concise approach to update the parameters dictionary, reducing the complexity of the code and improving readability.
I was able to adjust the settings on mobile so you shouldn't have to wait for manual ci runs 👍 |
|
Sorry, but it's hard to follow our conversation with code rabbit producing a lot verbiage. I'm not quite understanding the Lint Commit Messages thing. |
Don't worry about the commits as only the title matters since it will get squashed at the end anyways |
bdraco
changed the title
|
Alright, the pyunifiprotect library went completely poof, so I'll be focusing 100% of my attention here now. :p Not sure what prompted the spontaneous deletion of the Christopher's fork of the library, but I see it was forked from you, which was forked from briis. Just wondering what the story is if you know/willing to share. I think it's safe to say that this is the official (unofficial) library for integrating with Unifi Protect now. |
This comment best summarizes the situation |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/uiprotect/api.py (6 hunks)
Additional context used
GitHub Check: codecov/patch
src/uiprotect/api.py
[warning] 1471-1471: src/uiprotect/api.py#L1471
Added line #L1471 was not covered by tests
[warning] 1473-1473: src/uiprotect/api.py#L1473
Added line #L1473 was not covered by tests
[warning] 1483-1483: src/uiprotect/api.py#L1483
Added line #L1483 was not covered by tests
[warning] 1489-1489: src/uiprotect/api.py#L1489
Added line #L1489 was not covered by tests
[warning] 1499-1499: src/uiprotect/api.py#L1499
Added line #L1499 was not covered by tests
[warning] 1501-1501: src/uiprotect/api.py#L1501
Added line #L1501 was not covered by tests
[warning] 1503-1503: src/uiprotect/api.py#L1503
Added line #L1503 was not covered by tests
[warning] 1505-1505: src/uiprotect/api.py#L1505
Added line #L1505 was not covered by tests
[warning] 1507-1507: src/uiprotect/api.py#L1507
Added line #L1507 was not covered by tests
[warning] 1513-1514: src/uiprotect/api.py#L1513-L1514
Added lines #L1513 - L1514 were not covered by tests
[warning] 1544-1545: src/uiprotect/api.py#L1544-L1545
Added lines #L1544 - L1545 were not covered by tests
[warning] 1554-1554: src/uiprotect/api.py#L1554
Added line #L1554 was not covered by tests
[warning] 1559-1560: src/uiprotect/api.py#L1559-L1560
Added lines #L1559 - L1560 were not covered by tests
[warning] 1624-1625: src/uiprotect/api.py#L1624-L1625
Added lines #L1624 - L1625 were not covered by tests
[warning] 1640-1640: src/uiprotect/api.py#L1640
Added line #L1640 was not covered by tests
[warning] 1701-1701: src/uiprotect/api.py#L1701
Added line #L1701 was not covered by tests
[warning] 1703-1703: src/uiprotect/api.py#L1703
Added line #L1703 was not covered by tests
|
I've been keeping this up to date with main. No rush, but if you want to get this change in before HA beta on Wednesday, it will need another turn before Monday. |
| async def prepare_camera_video( | ||
| self, | ||
| camera_id: str, | ||
| start: datetime, | ||
| end: datetime, | ||
| channel_index: int = 0, | ||
| validate_channel_id: bool = True, | ||
| fps: int | None = None, | ||
| filename: str | None = None, | ||
| ) -> list[Any] | dict[str, Any] | None: | ||
| """ | ||
| Prepares MP4 video from a given camera at a specific time. | ||
|
|
||
| This is the newer implementation of retrieving video from Unifi Protect. | ||
| When using this method, it should be followed up with download_camera_video(). | ||
|
|
||
| Start/End of video export are approximate. It may be +/- a few seconds. | ||
|
|
||
| Providing the `fps` parameter creates a "timelapse" export wtih the given FPS | ||
| value. Protect app gives the options for 60x (fps=4), 120x (fps=8), 300x | ||
| (fps=20), and 600x (fps=40). | ||
|
|
||
| You will receive a filename and an expiry time in seconds. | ||
| """ | ||
| if validate_channel_id and self._bootstrap is not None: | ||
| try: | ||
| camera = self._bootstrap.cameras[camera_id] | ||
| camera.channels[channel_index] | ||
| except IndexError as e: | ||
| raise BadRequest from e | ||
|
|
There was a problem hiding this comment.
Enhance error handling in prepare_camera_video method.
The method catches IndexError to handle invalid channel_index but does not account for other potential issues such as KeyError if camera_id is not valid. Improve the error handling to cover these cases.
- except IndexError as e:
- raise BadRequest from e
+ except (IndexError, KeyError) as e:
+ raise BadRequest(f"Invalid input: {str(e)}") from eCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def prepare_camera_video( | |
| self, | |
| camera_id: str, | |
| start: datetime, | |
| end: datetime, | |
| channel_index: int = 0, | |
| validate_channel_id: bool = True, | |
| fps: int | None = None, | |
| filename: str | None = None, | |
| ) -> list[Any] | dict[str, Any] | None: | |
| """ | |
| Prepares MP4 video from a given camera at a specific time. | |
| This is the newer implementation of retrieving video from Unifi Protect. | |
| When using this method, it should be followed up with download_camera_video(). | |
| Start/End of video export are approximate. It may be +/- a few seconds. | |
| Providing the `fps` parameter creates a "timelapse" export wtih the given FPS | |
| value. Protect app gives the options for 60x (fps=4), 120x (fps=8), 300x | |
| (fps=20), and 600x (fps=40). | |
| You will receive a filename and an expiry time in seconds. | |
| """ | |
| if validate_channel_id and self._bootstrap is not None: | |
| try: | |
| camera = self._bootstrap.cameras[camera_id] | |
| camera.channels[channel_index] | |
| except IndexError as e: | |
| raise BadRequest from e | |
| if validate_channel_id and self._bootstrap is not None: | |
| try: | |
| camera = self._bootstrap.cameras[camera_id] | |
| camera.channels[channel_index] | |
| except (IndexError, KeyError) as e: | |
| raise BadRequest(f"Invalid input: {str(e)}") from e |
I'll see what I can do. My time has been very limited these past few weeks and will continue to remain limited until late July. What's throwing me are the tests, since I can't quite seem to figure out the flow of logic that is letting make the proper tests for it. I've been predominantly developing in PHP and have never dabbled with pytest before. |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
src/uiprotect/api.py (1)
Line range hint
1607-1751: Refactor the deprecatedget_camera_videomethod to improve error handlingCurrently, the method catches a broad
Exception, which could mask other important exceptions that should be handled specifically. Narrow down the exceptions to those that are expected to occur.- except Exception: + except (BadRequest, NvrError) as e: + _LOGGER.error(f"Error during video retrieval: {str(e)}") + raiseTools
GitHub Check: codecov/patch
[warning] 1729-1729: src/uiprotect/api.py#L1729
Added line #L1729 was not covered by tests
[warning] 1731-1731: src/uiprotect/api.py#L1731
Added line #L1731 was not covered by tests
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/uiprotect/api.py (6 hunks)
Additional context used
GitHub Check: codecov/patch
src/uiprotect/api.py
[warning] 1499-1499: src/uiprotect/api.py#L1499
Added line #L1499 was not covered by tests
[warning] 1501-1501: src/uiprotect/api.py#L1501
Added line #L1501 was not covered by tests
[warning] 1511-1511: src/uiprotect/api.py#L1511
Added line #L1511 was not covered by tests
[warning] 1517-1517: src/uiprotect/api.py#L1517
Added line #L1517 was not covered by tests
[warning] 1527-1527: src/uiprotect/api.py#L1527
Added line #L1527 was not covered by tests
[warning] 1529-1529: src/uiprotect/api.py#L1529
Added line #L1529 was not covered by tests
[warning] 1531-1531: src/uiprotect/api.py#L1531
Added line #L1531 was not covered by tests
[warning] 1533-1533: src/uiprotect/api.py#L1533
Added line #L1533 was not covered by tests
[warning] 1535-1535: src/uiprotect/api.py#L1535
Added line #L1535 was not covered by tests
[warning] 1541-1542: src/uiprotect/api.py#L1541-L1542
Added lines #L1541 - L1542 were not covered by tests
[warning] 1572-1573: src/uiprotect/api.py#L1572-L1573
Added lines #L1572 - L1573 were not covered by tests
[warning] 1582-1582: src/uiprotect/api.py#L1582
Added line #L1582 was not covered by tests
[warning] 1587-1588: src/uiprotect/api.py#L1587-L1588
Added lines #L1587 - L1588 were not covered by tests
[warning] 1652-1653: src/uiprotect/api.py#L1652-L1653
Added lines #L1652 - L1653 were not covered by tests
[warning] 1668-1668: src/uiprotect/api.py#L1668
Added line #L1668 was not covered by tests
[warning] 1729-1729: src/uiprotect/api.py#L1729
Added line #L1729 was not covered by tests
[warning] 1731-1731: src/uiprotect/api.py#L1731
Added line #L1731 was not covered by tests
| async def download_camera_video( | ||
| self, | ||
| camera_id: str, | ||
| filename: str, | ||
| output_file: Path | None = None, | ||
| iterator_callback: IteratorCallback | None = None, | ||
| progress_callback: ProgressCallback | None = None, | ||
| chunk_size: int = 65536, | ||
| ) -> bytes: | ||
| """ | ||
| Downloads a prepared MP4 video from a given camera. | ||
|
|
||
| This is the newer implementation of retrieving video from Unifi Protect. | ||
| You need to supply the filename returned from prepare_camera_video(). | ||
|
|
||
| It is recommended to provide an output file or progress callback for larger | ||
| video clips, otherwise the full video must be downloaded to memory before | ||
| being written. | ||
| """ | ||
| path = "video/download" | ||
|
|
||
| params = { | ||
| "camera": camera_id, | ||
| "filename": filename, | ||
| } | ||
|
|
||
| if ( | ||
| iterator_callback is None | ||
| and progress_callback is None | ||
| and output_file is None | ||
| ): | ||
| return await self.api_request_raw( | ||
| path, | ||
| params=params, | ||
| raise_exception=False, | ||
| ) |
There was a problem hiding this comment.
Potential memory usage issue when downloading large video files
The method download_camera_video allows downloading large video files directly into memory if neither iterator_callback, progress_callback, nor output_file are provided. This can lead to high memory usage which might cause performance issues or crashes, especially for large video files.
- if (iterator_callback is None and progress_callback is None and output_file is None):
+ if output_file is None:
+ temp_file = Path(tempfile.mkstemp()[1])
+ result = await self.download_camera_video(
+ camera_id=camera_id,
+ filename=filename,
+ output_file=temp_file,
+ iterator_callback=iterator_callback,
+ progress_callback=progress_callback,
+ chunk_size=chunk_size,
+ )
+ with open(temp_file, 'rb') as file:
+ return file.read()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def download_camera_video( | |
| self, | |
| camera_id: str, | |
| filename: str, | |
| output_file: Path | None = None, | |
| iterator_callback: IteratorCallback | None = None, | |
| progress_callback: ProgressCallback | None = None, | |
| chunk_size: int = 65536, | |
| ) -> bytes: | |
| """ | |
| Downloads a prepared MP4 video from a given camera. | |
| This is the newer implementation of retrieving video from Unifi Protect. | |
| You need to supply the filename returned from prepare_camera_video(). | |
| It is recommended to provide an output file or progress callback for larger | |
| video clips, otherwise the full video must be downloaded to memory before | |
| being written. | |
| """ | |
| path = "video/download" | |
| params = { | |
| "camera": camera_id, | |
| "filename": filename, | |
| } | |
| if ( | |
| iterator_callback is None | |
| and progress_callback is None | |
| and output_file is None | |
| ): | |
| return await self.api_request_raw( | |
| path, | |
| params=params, | |
| raise_exception=False, | |
| ) | |
| async def download_camera_video( | |
| self, | |
| camera_id: str, | |
| filename: str, | |
| output_file: Path | None = None, | |
| iterator_callback: IteratorCallback | None = None, | |
| progress_callback: ProgressCallback | None = None, | |
| chunk_size: int = 65536, | |
| ) -> bytes: | |
| """ | |
| Downloads a prepared MP4 video from a given camera. | |
| This is the newer implementation of retrieving video from Unifi Protect. | |
| You need to supply the filename returned from prepare_camera_video(). | |
| It is recommended to provide an output file or progress callback for larger | |
| video clips, otherwise the full video must be downloaded to memory before | |
| being written. | |
| """ | |
| path = "video/download" | |
| params = { | |
| "camera": camera_id, | |
| "filename": filename, | |
| } | |
| if output_file is None: | |
| temp_file = Path(tempfile.mkstemp()[1]) | |
| result = await self.download_camera_video( | |
| camera_id=camera_id, | |
| filename=filename, | |
| output_file=temp_file, | |
| iterator_callback=iterator_callback, | |
| progress_callback=progress_callback, | |
| chunk_size=chunk_size, | |
| ) | |
| with open(temp_file, 'rb') as file: | |
| return file.read() |
Tools
GitHub Check: codecov/patch
[warning] 1499-1499: src/uiprotect/api.py#L1499
Added line #L1499 was not covered by tests
[warning] 1501-1501: src/uiprotect/api.py#L1501
Added line #L1501 was not covered by tests
[warning] 1511-1511: src/uiprotect/api.py#L1511
Added line #L1511 was not covered by tests
No worries. Take your time. |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
src/uiprotect/api.py (1)
Line range hint
1607-1751: Enhance error handling and optimize memory usage inexport_camera_video.The method lacks explicit error handling for network requests and file operations, which could lead to unhandled exceptions. Additionally, downloading large video files directly into memory can lead to high memory usage. Consider the following improvements:
- Add error handling to manage potential failures in network requests or file operations.
- Optimize memory usage by always writing to a temporary file if
output_fileis not provided.async def export_camera_video( self, camera_id: str, start: datetime, end: datetime, channel_index: int = 0, validate_channel_id: bool = True, output_file: Path | None = None, iterator_callback: IteratorCallback | None = None, progress_callback: ProgressCallback | None = None, chunk_size: int = 65536, fps: int | None = None, ) -> bytes | None: if validate_channel_id and self._bootstrap is not None: try: camera = self._bootstrap.cameras[camera_id] camera.channels[channel_index] except (IndexError, KeyError, AttributeError) as e: raise BadRequest(f"Invalid input: {str(e)}") from e params = { "camera": camera_id, "start": to_js_time(start), "end": to_js_time(end), } if channel_index == 3: params.update({"lens": 2}) else: params.update({"channel": channel_index}) if fps is not None and fps > 0: params["fps"] = fps params["type"] = "timelapse" else: params["type"] = "rotating" path = "video/export" if ( iterator_callback is None and progress_callback is None and output_file is None ): return await self.api_request_raw( path, params=params, raise_exception=False, ) try: r = await self._os.request( "get", urljoin(self.api_path, path), auto_close=False, timeout=0, params=params, ) if output_file is not None: async with aiofiles.open(output_file, "wb") as output: async def callback(total: int, chunk: bytes | None) -> None: if iterator_callback is not None: await iterator_callback(total, chunk) if chunk is not None: await output.write(chunk) await self._stream_response(r, chunk_size, callback, progress_callback) else: await self._stream_response( r, chunk_size, iterator_callback, progress_callback, ) r.close() except Exception as e: _LOGGER.error(f"Failed to export video: {str(e)}") raise return None
| async def get_camera_video( | ||
| self, | ||
| camera_id: str, | ||
| start: datetime, | ||
| end: datetime, | ||
| channel_index: int = 0, | ||
| validate_channel_id: bool = True, | ||
| output_file: Path | None = None, | ||
| iterator_callback: IteratorCallback | None = None, | ||
| progress_callback: ProgressCallback | None = None, | ||
| chunk_size: int = 65536, | ||
| fps: int | None = None, | ||
| filename: str | None = None, | ||
| ) -> bytes: | ||
| """ | ||
| Deprecated: maintained for backwards compatibility. | ||
|
|
||
| If you are using Unifi Protect 4 or later, please use | ||
| prepare_camera_video() and download_camera_video() instead. | ||
| """ | ||
| try: | ||
| prepare_response = await self.prepare_camera_video( | ||
| camera_id=camera_id, | ||
| start=start, | ||
| end=end, | ||
| channel_index=channel_index, | ||
| validate_channel_id=validate_channel_id, | ||
| fps=fps, | ||
| filename=filename, | ||
| ) | ||
|
|
||
| if isinstance(prepare_response, dict): | ||
| download_filename = prepare_response["fileName"] | ||
| else: | ||
| raise Exception | ||
|
|
||
| return await self.download_camera_video( | ||
| camera_id=camera_id, | ||
| filename=download_filename, | ||
| output_file=output_file, | ||
| iterator_callback=iterator_callback, | ||
| progress_callback=progress_callback, | ||
| chunk_size=chunk_size, | ||
| ) | ||
| except Exception: | ||
| return await self.export_camera_video( | ||
| camera_id=camera_id, | ||
| start=start, | ||
| end=end, | ||
| channel_index=channel_index, | ||
| validate_channel_id=validate_channel_id, | ||
| output_file=output_file, | ||
| iterator_callback=iterator_callback, | ||
| progress_callback=progress_callback, | ||
| chunk_size=chunk_size, | ||
| fps=fps, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Narrow down exceptions and add logging in get_camera_video.
The method catches a broad Exception, which could mask other important exceptions that should be handled specifically. Narrow down the exceptions to those that are expected to occur. Additionally, add logging to capture fallback scenarios for better debugging.
async def get_camera_video(
self,
camera_id: str,
start: datetime,
end: datetime,
channel_index: int = 0,
validate_channel_id: bool = True,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
fps: int | None = None,
filename: str | None = None,
) -> bytes | None:
"""
Deprecated: maintained for backwards compatibility.
If you are using Unifi Protect 4 or later, please use
prepare_camera_video() and download_camera_video() instead.
"""
try:
prepare_response = await self.prepare_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
fps=fps,
filename=filename,
)
if isinstance(prepare_response, dict):
download_filename = prepare_response["fileName"]
else:
raise BadRequest("Invalid response from prepare_camera_video")
return await self.download_camera_video(
camera_id=camera_id,
filename=download_filename,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
)
except (BadRequest, NvrError) as e:
_LOGGER.error(f"Error during video retrieval: {str(e)}")
return await self.export_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
fps=fps,
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def get_camera_video( | |
| self, | |
| camera_id: str, | |
| start: datetime, | |
| end: datetime, | |
| channel_index: int = 0, | |
| validate_channel_id: bool = True, | |
| output_file: Path | None = None, | |
| iterator_callback: IteratorCallback | None = None, | |
| progress_callback: ProgressCallback | None = None, | |
| chunk_size: int = 65536, | |
| fps: int | None = None, | |
| filename: str | None = None, | |
| ) -> bytes: | |
| """ | |
| Deprecated: maintained for backwards compatibility. | |
| If you are using Unifi Protect 4 or later, please use | |
| prepare_camera_video() and download_camera_video() instead. | |
| """ | |
| try: | |
| prepare_response = await self.prepare_camera_video( | |
| camera_id=camera_id, | |
| start=start, | |
| end=end, | |
| channel_index=channel_index, | |
| validate_channel_id=validate_channel_id, | |
| fps=fps, | |
| filename=filename, | |
| ) | |
| if isinstance(prepare_response, dict): | |
| download_filename = prepare_response["fileName"] | |
| else: | |
| raise Exception | |
| return await self.download_camera_video( | |
| camera_id=camera_id, | |
| filename=download_filename, | |
| output_file=output_file, | |
| iterator_callback=iterator_callback, | |
| progress_callback=progress_callback, | |
| chunk_size=chunk_size, | |
| ) | |
| except Exception: | |
| return await self.export_camera_video( | |
| camera_id=camera_id, | |
| start=start, | |
| end=end, | |
| channel_index=channel_index, | |
| validate_channel_id=validate_channel_id, | |
| output_file=output_file, | |
| iterator_callback=iterator_callback, | |
| progress_callback=progress_callback, | |
| chunk_size=chunk_size, | |
| fps=fps, | |
| ) | |
| async def get_camera_video( | |
| self, | |
| camera_id: str, | |
| start: datetime, | |
| end: datetime, | |
| channel_index: int = 0, | |
| validate_channel_id: bool = True, | |
| output_file: Path | None = None, | |
| iterator_callback: IteratorCallback | None = None, | |
| progress_callback: ProgressCallback | None = None, | |
| chunk_size: int = 65536, | |
| fps: int | None = None, | |
| filename: str | None = None, | |
| ) -> bytes | None: | |
| """ | |
| Deprecated: maintained for backwards compatibility. | |
| If you are using Unifi Protect 4 or later, please use | |
| prepare_camera_video() and download_camera_video() instead. | |
| """ | |
| try: | |
| prepare_response = await self.prepare_camera_video( | |
| camera_id=camera_id, | |
| start=start, | |
| end=end, | |
| channel_index=channel_index, | |
| validate_channel_id=validate_channel_id, | |
| fps=fps, | |
| filename=filename, | |
| ) | |
| if isinstance(prepare_response, dict): | |
| download_filename = prepare_response["fileName"] | |
| else: | |
| raise BadRequest("Invalid response from prepare_camera_video") | |
| return await self.download_camera_video( | |
| camera_id=camera_id, | |
| filename=download_filename, | |
| output_file=output_file, | |
| iterator_callback=iterator_callback, | |
| progress_callback=progress_callback, | |
| chunk_size=chunk_size, | |
| ) | |
| except (BadRequest, NvrError) as e: | |
| _LOGGER.error(f"Error during video retrieval: {str(e)}") | |
| return await self.export_camera_video( | |
| camera_id=camera_id, | |
| start=start, | |
| end=end, | |
| channel_index=channel_index, | |
| validate_channel_id=validate_channel_id, | |
| output_file=output_file, | |
| iterator_callback=iterator_callback, | |
| progress_callback=progress_callback, | |
| chunk_size=chunk_size, | |
| fps=fps, | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
src/uiprotect/api.py (1)
Line range hint
1605-1749: Enhance error handling and optimize handling offpsparameter inexport_camera_video.
Error Handling: Similar to
prepare_camera_video, ensure that thecameraobject and itschannelsattribute are valid before accessing them. This can preventAttributeErrororKeyError.Optimizing
fpsHandling: The handling of thefpsparameter can be optimized by checking iffpsis notNonebefore setting theparams['type']. This avoids unnecessary dictionary operations.async def export_camera_video( self, camera_id: str, start: datetime, end: datetime, channel_index: int = 0, validate_channel_id: bool = True, output_file: Path | None = None, iterator_callback: IteratorCallback | None = None, progress_callback: ProgressCallback | None = None, chunk_size: int = 65536, fps: int | None = None, ) -> bytes: if validate_channel_id and self._bootstrap is not None: try: camera = self._bootstrap.cameras[camera_id] camera.channels[channel_index] except IndexError as e: raise BadRequest from e params = { "camera": camera_id, "start": to_js_time(start), "end": to_js_time(end), } if channel_index == 3: params.update({"lens": 2}) else: params.update({"channel": channel_index}) if fps is not None and fps > 0: params["fps"] = fps params["type"] = "timelapse" else: params["type"] = "rotating" path = "video/export" if ( iterator_callback is None and progress_callback is None and output_file is None ): return await self.api_request_raw( path, params=params, raise_exception=False, ) r = await self._os.request( "get", urljoin(self.api_path, path), auto_close=False, timeout=0, params=params, ) if output_file is not None: async with aiofiles.open(output_file, "wb") as output: async def callback(total: int, chunk: bytes | None) -> None: if iterator_callback is not None: await iterator_callback(total, chunk) if chunk is not None: await output.write(chunk) await self._stream_response(r, chunk_size, callback, progress_callback) else: await self._stream_response( r, chunk_size, iterator_callback, progress_callback, ) r.close() return None
| async def download_camera_video( | ||
| self, | ||
| camera_id: str, | ||
| filename: str, | ||
| output_file: Path | None = None, | ||
| iterator_callback: IteratorCallback | None = None, | ||
| progress_callback: ProgressCallback | None = None, | ||
| chunk_size: int = 65536, | ||
| ) -> bytes: | ||
| """ | ||
| Downloads a prepared MP4 video from a given camera. | ||
|
|
||
| This is the newer implementation of retrieving video from Unifi Protect. | ||
| You need to supply the filename returned from prepare_camera_video(). | ||
|
|
||
| It is recommended to provide an output file or progress callback for larger | ||
| video clips, otherwise the full video must be downloaded to memory before | ||
| being written. | ||
| """ | ||
| path = "video/download" | ||
|
|
||
| params = { | ||
| "camera": camera_id, | ||
| "filename": filename, | ||
| } | ||
|
|
||
| if ( | ||
| iterator_callback is None | ||
| and progress_callback is None | ||
| and output_file is None | ||
| ): | ||
| return await self.api_request_raw( | ||
| path, | ||
| params=params, | ||
| raise_exception=False, | ||
| ) | ||
|
|
||
| r = await self._os.request( | ||
| "get", | ||
| urljoin(self.api_path, path), | ||
| auto_close=False, | ||
| timeout=0, | ||
| params=params, | ||
| ) | ||
| if output_file is not None: | ||
| async with aiofiles.open(output_file, "wb") as output: | ||
|
|
||
| async def callback(total: int, chunk: bytes | None) -> None: | ||
| if iterator_callback is not None: | ||
| await iterator_callback(total, chunk) | ||
| if chunk is not None: | ||
| await output.write(chunk) | ||
|
|
||
| await self._stream_response(r, chunk_size, callback, progress_callback) | ||
| else: | ||
| await self._stream_response( | ||
| r, | ||
| chunk_size, | ||
| iterator_callback, | ||
| progress_callback, | ||
| ) | ||
| r.close() | ||
| return None |
There was a problem hiding this comment.
Optimize memory usage and error handling in download_camera_video.
-
Memory Usage: When neither
iterator_callback,progress_callback, noroutput_fileare provided, the method downloads the entire video into memory. This could lead to high memory usage, especially for large video files. Consider always writing to a temporary file ifoutput_fileis not provided to avoid high memory consumption. -
Error Handling: The method lacks explicit error handling for network requests and file operations, which could lead to unhandled exceptions. Consider adding error handling to manage potential failures in network requests or file operations.
-
Resource Management: Ensure that the response object
ris always properly closed, even in the case of exceptions. This can be achieved by using a context manager or ensuringr.close()is called in afinallyblock.
async def download_camera_video(
self,
camera_id: str,
filename: str,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
) -> bytes | None:
path = "video/download"
params = {
"camera": camera_id,
"filename": filename,
}
if (
iterator_callback is None
and progress_callback is None
and output_file is None
):
return await self.api_request_raw(
path,
params=params,
raise_exception=False,
)
try:
r = await self._os.request(
"get",
urljoin(self.api_path, path),
auto_close=False,
timeout=0,
params=params,
)
if output_file is not None:
async with aiofiles.open(output_file, "wb") as output:
async def callback(total: int, chunk: bytes | None) -> None:
if iterator_callback is not None:
await iterator_callback(total, chunk)
if chunk is not None:
await output.write(chunk)
await self._stream_response(r, chunk_size, callback, progress_callback)
else:
await self._stream_response(
r,
chunk_size,
iterator_callback,
progress_callback,
)
r.close()
except Exception as e:
_LOGGER.error(f"Failed to download video: {str(e)}")
raise
return NoneCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def download_camera_video( | |
| self, | |
| camera_id: str, | |
| filename: str, | |
| output_file: Path | None = None, | |
| iterator_callback: IteratorCallback | None = None, | |
| progress_callback: ProgressCallback | None = None, | |
| chunk_size: int = 65536, | |
| ) -> bytes: | |
| """ | |
| Downloads a prepared MP4 video from a given camera. | |
| This is the newer implementation of retrieving video from Unifi Protect. | |
| You need to supply the filename returned from prepare_camera_video(). | |
| It is recommended to provide an output file or progress callback for larger | |
| video clips, otherwise the full video must be downloaded to memory before | |
| being written. | |
| """ | |
| path = "video/download" | |
| params = { | |
| "camera": camera_id, | |
| "filename": filename, | |
| } | |
| if ( | |
| iterator_callback is None | |
| and progress_callback is None | |
| and output_file is None | |
| ): | |
| return await self.api_request_raw( | |
| path, | |
| params=params, | |
| raise_exception=False, | |
| ) | |
| r = await self._os.request( | |
| "get", | |
| urljoin(self.api_path, path), | |
| auto_close=False, | |
| timeout=0, | |
| params=params, | |
| ) | |
| if output_file is not None: | |
| async with aiofiles.open(output_file, "wb") as output: | |
| async def callback(total: int, chunk: bytes | None) -> None: | |
| if iterator_callback is not None: | |
| await iterator_callback(total, chunk) | |
| if chunk is not None: | |
| await output.write(chunk) | |
| await self._stream_response(r, chunk_size, callback, progress_callback) | |
| else: | |
| await self._stream_response( | |
| r, | |
| chunk_size, | |
| iterator_callback, | |
| progress_callback, | |
| ) | |
| r.close() | |
| return None | |
| async def download_camera_video( | |
| self, | |
| camera_id: str, | |
| filename: str, | |
| output_file: Path | None = None, | |
| iterator_callback: IteratorCallback | None = None, | |
| progress_callback: ProgressCallback | None = None, | |
| chunk_size: int = 65536, | |
| ) -> bytes | None: | |
| path = "video/download" | |
| params = { | |
| "camera": camera_id, | |
| "filename": filename, | |
| } | |
| if ( | |
| iterator_callback is None | |
| and progress_callback is None | |
| and output_file is None | |
| ): | |
| return await self.api_request_raw( | |
| path, | |
| params=params, | |
| raise_exception=False, | |
| ) | |
| try: | |
| r = await self._os.request( | |
| "get", | |
| urljoin(self.api_path, path), | |
| auto_close=False, | |
| timeout=0, | |
| params=params, | |
| ) | |
| if output_file is not None: | |
| async with aiofiles.open(output_file, "wb") as output: | |
| async def callback(total: int, chunk: bytes | None) -> None: | |
| if iterator_callback is not None: | |
| await iterator_callback(total, chunk) | |
| if chunk is not None: | |
| await output.write(chunk) | |
| await self._stream_response(r, chunk_size, callback, progress_callback) | |
| else: | |
| await self._stream_response( | |
| r, | |
| chunk_size, | |
| iterator_callback, | |
| progress_callback, | |
| ) | |
| r.close() | |
| except Exception as e: | |
| _LOGGER.error(f"Failed to download video: {str(e)}") | |
| raise | |
| return None |
|
Has there been any progress on this? Its blocking unifi-protect-backup from being swapped to this library. If there is no plan to progress this PR, I might have to drop support for this new download method in order to make the switch |
|
@ep1cman If you want, I can give you access to this repo so you can finish this PR |
So sorry, I just have not had any time. I anticipate my time freeing up towards the end of July where I can then focus on this once again. The only thing actually holding up this PR, is implementing some test cases, which I'm not too familiar with in Python. If you know how to set those up, that would get this PR over the finish line. |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (1)
src/uiprotect/api.py (1)
Line range hint
1610-1657: Optimizefpshandling inexport_camera_video.The handling of the
fpsparameter can be optimized by checking iffpsis notNonebefore settingparams['type']. This avoids unnecessary dictionary operations.Here's a proposed improvement:
if fps is not None and fps > 0: params["fps"] = fps params["type"] = "timelapse" else: params["type"] = "rotating"Additionally, the function lacks test coverage for certain lines, as indicated by static analysis.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/uiprotect/api.py (6 hunks)
Additional context used
GitHub Check: codecov/patch
src/uiprotect/api.py
[warning] 1502-1502: src/uiprotect/api.py#L1502
Added line #L1502 was not covered by tests
[warning] 1504-1504: src/uiprotect/api.py#L1504
Added line #L1504 was not covered by tests
[warning] 1509-1509: src/uiprotect/api.py#L1509
Added line #L1509 was not covered by tests
[warning] 1514-1514: src/uiprotect/api.py#L1514
Added line #L1514 was not covered by tests
[warning] 1520-1520: src/uiprotect/api.py#L1520
Added line #L1520 was not covered by tests
[warning] 1527-1528: src/uiprotect/api.py#L1527-L1528
Added lines #L1527 - L1528 were not covered by tests
[warning] 1530-1534: src/uiprotect/api.py#L1530-L1534
Added lines #L1530 - L1534 were not covered by tests
[warning] 1536-1536: src/uiprotect/api.py#L1536
Added line #L1536 was not covered by tests
[warning] 1538-1538: src/uiprotect/api.py#L1538
Added line #L1538 was not covered by tests
[warning] 1544-1545: src/uiprotect/api.py#L1544-L1545
Added lines #L1544 - L1545 were not covered by tests
[warning] 1575-1576: src/uiprotect/api.py#L1575-L1576
Added lines #L1575 - L1576 were not covered by tests
[warning] 1585-1585: src/uiprotect/api.py#L1585
Added line #L1585 was not covered by tests
[warning] 1590-1591: src/uiprotect/api.py#L1590-L1591
Added lines #L1590 - L1591 were not covered by tests
[warning] 1655-1656: src/uiprotect/api.py#L1655-L1656
Added lines #L1655 - L1656 were not covered by tests
[warning] 1671-1671: src/uiprotect/api.py#L1671
Added line #L1671 was not covered by tests
[warning] 1732-1732: src/uiprotect/api.py#L1732
Added line #L1732 was not covered by tests
[warning] 1734-1734: src/uiprotect/api.py#L1734
Added line #L1734 was not covered by tests
| async def download_camera_video( | ||
| self, | ||
| camera_id: str, | ||
| filename: str, | ||
| output_file: Path | None = None, | ||
| iterator_callback: IteratorCallback | None = None, | ||
| progress_callback: ProgressCallback | None = None, | ||
| chunk_size: int = 65536, | ||
| ) -> bytes: | ||
| """ | ||
| Downloads a prepared MP4 video from a given camera. | ||
|
|
||
| This is the newer implementation of retrieving video from Unifi Protect. | ||
| You need to supply the filename returned from prepare_camera_video(). | ||
|
|
||
| It is recommended to provide an output file or progress callback for larger | ||
| video clips, otherwise the full video must be downloaded to memory before | ||
| being written. | ||
| """ |
There was a problem hiding this comment.
Enhance error handling and optimize memory usage in download_camera_video.
- Memory Usage: Consider always writing to a temporary file if
output_fileis not provided to avoid high memory consumption. - Error Handling: Add error handling to manage potential failures in network requests or file operations.
- Test Coverage: The function lacks test coverage for several lines, as indicated by static analysis.
Here's a proposed improvement for handling memory usage:
import tempfile
from pathlib import Path
async def download_camera_video(
self,
camera_id: str,
filename: str,
output_file: Path | None = None,
iterator_callback: IteratorCallback | None = None,
progress_callback: ProgressCallback | None = None,
chunk_size: int = 65536,
) -> bytes:
if output_file is None:
temp_file = Path(tempfile.mkstemp()[1])
await self._download_to_file(camera_id, filename, temp_file, iterator_callback, progress_callback, chunk_size)
with open(temp_file, 'rb') as file:
return file.read()
else:
await self._download_to_file(camera_id, filename, output_file, iterator_callback, progress_callback, chunk_size)
return None
async def _download_to_file(
self,
camera_id: str,
filename: str,
output_file: Path,
iterator_callback: IteratorCallback | None,
progress_callback: ProgressCallback | None,
chunk_size: int
) -> None:
# Existing logic for downloading to a file| async def get_camera_video( | ||
| self, | ||
| camera_id: str, | ||
| start: datetime, | ||
| end: datetime, | ||
| channel_index: int = 0, | ||
| validate_channel_id: bool = True, | ||
| output_file: Path | None = None, | ||
| iterator_callback: IteratorCallback | None = None, | ||
| progress_callback: ProgressCallback | None = None, | ||
| chunk_size: int = 65536, | ||
| fps: int | None = None, | ||
| filename: str | None = None, | ||
| ) -> bytes: | ||
| """ | ||
| Deprecated: maintained for backwards compatibility. | ||
|
|
||
| If you are using Unifi Protect 4 or later, please use | ||
| prepare_camera_video() and download_camera_video() instead. | ||
| """ | ||
| try: | ||
| prepare_response = await self.prepare_camera_video( | ||
| camera_id=camera_id, | ||
| start=start, | ||
| end=end, | ||
| channel_index=channel_index, | ||
| validate_channel_id=validate_channel_id, | ||
| fps=fps, | ||
| filename=filename, | ||
| ) | ||
|
|
||
| if isinstance(prepare_response, dict): | ||
| download_filename = prepare_response["fileName"] | ||
| else: | ||
| raise Exception | ||
|
|
||
| return await self.download_camera_video( | ||
| camera_id=camera_id, | ||
| filename=download_filename, | ||
| output_file=output_file, | ||
| iterator_callback=iterator_callback, | ||
| progress_callback=progress_callback, | ||
| chunk_size=chunk_size, | ||
| ) | ||
| except Exception: | ||
| return await self.export_camera_video( | ||
| camera_id=camera_id, | ||
| start=start, | ||
| end=end, | ||
| channel_index=channel_index, | ||
| validate_channel_id=validate_channel_id, | ||
| output_file=output_file, | ||
| iterator_callback=iterator_callback, | ||
| progress_callback=progress_callback, | ||
| chunk_size=chunk_size, | ||
| fps=fps, | ||
| ) |
There was a problem hiding this comment.
Narrow down exceptions and add logging in get_camera_video.
- Exception Handling: Narrow down the exceptions to those that are expected to occur, such as
BadRequestorNvrError. This prevents masking other important exceptions. - Logging: Add logging to capture fallback scenarios for better debugging.
- Test Coverage: The function lacks test coverage for certain lines, as indicated by static analysis.
Here's a proposed improvement:
try:
prepare_response = await self.prepare_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
fps=fps,
filename=filename,
)
if isinstance(prepare_response, dict):
download_filename = prepare_response["fileName"]
else:
raise BadRequest("Invalid response from prepare_camera_video")
return await self.download_camera_video(
camera_id=camera_id,
filename=download_filename,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
)
except (BadRequest, NvrError) as e:
_LOGGER.error(f"Error during video retrieval: {str(e)}")
return await self.export_camera_video(
camera_id=camera_id,
start=start,
end=end,
channel_index=channel_index,
validate_channel_id=validate_channel_id,
output_file=output_file,
iterator_callback=iterator_callback,
progress_callback=progress_callback,
chunk_size=chunk_size,
fps=fps,
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def get_camera_video( | |
| self, | |
| camera_id: str, | |
| start: datetime, | |
| end: datetime, | |
| channel_index: int = 0, | |
| validate_channel_id: bool = True, | |
| output_file: Path | None = None, | |
| iterator_callback: IteratorCallback | None = None, | |
| progress_callback: ProgressCallback | None = None, | |
| chunk_size: int = 65536, | |
| fps: int | None = None, | |
| filename: str | None = None, | |
| ) -> bytes: | |
| """ | |
| Deprecated: maintained for backwards compatibility. | |
| If you are using Unifi Protect 4 or later, please use | |
| prepare_camera_video() and download_camera_video() instead. | |
| """ | |
| try: | |
| prepare_response = await self.prepare_camera_video( | |
| camera_id=camera_id, | |
| start=start, | |
| end=end, | |
| channel_index=channel_index, | |
| validate_channel_id=validate_channel_id, | |
| fps=fps, | |
| filename=filename, | |
| ) | |
| if isinstance(prepare_response, dict): | |
| download_filename = prepare_response["fileName"] | |
| else: | |
| raise Exception | |
| return await self.download_camera_video( | |
| camera_id=camera_id, | |
| filename=download_filename, | |
| output_file=output_file, | |
| iterator_callback=iterator_callback, | |
| progress_callback=progress_callback, | |
| chunk_size=chunk_size, | |
| ) | |
| except Exception: | |
| return await self.export_camera_video( | |
| camera_id=camera_id, | |
| start=start, | |
| end=end, | |
| channel_index=channel_index, | |
| validate_channel_id=validate_channel_id, | |
| output_file=output_file, | |
| iterator_callback=iterator_callback, | |
| progress_callback=progress_callback, | |
| chunk_size=chunk_size, | |
| fps=fps, | |
| ) | |
| async def get_camera_video( | |
| self, | |
| camera_id: str, | |
| start: datetime, | |
| end: datetime, | |
| channel_index: int = 0, | |
| validate_channel_id: bool = True, | |
| output_file: Path | None = None, | |
| iterator_callback: IteratorCallback | None = None, | |
| progress_callback: ProgressCallback | None = None, | |
| chunk_size: int = 65536, | |
| fps: int | None = None, | |
| filename: str | None = None, | |
| ) -> bytes: | |
| """ | |
| Deprecated: maintained for backwards compatibility. | |
| If you are using Unifi Protect 4 or later, please use | |
| prepare_camera_video() and download_camera_video() instead. | |
| """ | |
| try: | |
| prepare_response = await self.prepare_camera_video( | |
| camera_id=camera_id, | |
| start=start, | |
| end=end, | |
| channel_index=channel_index, | |
| validate_channel_id=validate_channel_id, | |
| fps=fps, | |
| filename=filename, | |
| ) | |
| if isinstance(prepare_response, dict): | |
| download_filename = prepare_response["fileName"] | |
| else: | |
| raise BadRequest("Invalid response from prepare_camera_video") | |
| return await self.download_camera_video( | |
| camera_id=camera_id, | |
| filename=download_filename, | |
| output_file=output_file, | |
| iterator_callback=iterator_callback, | |
| progress_callback=progress_callback, | |
| chunk_size=chunk_size, | |
| ) | |
| except (BadRequest, NvrError) as e: | |
| _LOGGER.error(f"Error during video retrieval: {str(e)}") | |
| return await self.export_camera_video( | |
| camera_id=camera_id, | |
| start=start, | |
| end=end, | |
| channel_index=channel_index, | |
| validate_channel_id=validate_channel_id, | |
| output_file=output_file, | |
| iterator_callback=iterator_callback, | |
| progress_callback=progress_callback, | |
| chunk_size=chunk_size, | |
| fps=fps, | |
| ) |
Tools
GitHub Check: codecov/patch
[warning] 1732-1732: src/uiprotect/api.py#L1732
Added line #L1732 was not covered by tests
[warning] 1734-1734: src/uiprotect/api.py#L1734
Added line #L1734 was not covered by tests
| async def prepare_camera_video( | ||
| self, | ||
| camera_id: str, | ||
| start: datetime, | ||
| end: datetime, | ||
| channel_index: int = 0, | ||
| validate_channel_id: bool = True, | ||
| fps: int | None = None, | ||
| filename: str | None = None, | ||
| ) -> list[Any] | dict[str, Any] | None: | ||
| """ | ||
| Prepares MP4 video from a given camera at a specific time. | ||
|
|
||
| This is the newer implementation of retrieving video from Unifi Protect. | ||
| When using this method, it should be followed up with download_camera_video(). | ||
|
|
||
| Start/End of video export are approximate. It may be +/- a few seconds. | ||
|
|
||
| Providing the `fps` parameter creates a "timelapse" export wtih the given FPS | ||
| value. Protect app gives the options for 60x (fps=4), 120x (fps=8), 300x | ||
| (fps=20), and 600x (fps=40). | ||
|
|
||
| You will receive a filename and an expiry time in seconds. | ||
| """ | ||
| if validate_channel_id and self._bootstrap is not None: | ||
| try: | ||
| camera = self._bootstrap.cameras[camera_id] | ||
| camera.channels[channel_index] | ||
| except IndexError as e: | ||
| raise BadRequest from e | ||
|
|
||
| params = { | ||
| "camera": camera_id, | ||
| "start": to_js_time(start), | ||
| "end": to_js_time(end), | ||
| } | ||
|
|
||
| if channel_index == 3: | ||
| params.update({"lens": 2}) | ||
| else: | ||
| params.update({"channel": channel_index}) | ||
|
|
||
| if fps is not None and fps > 0: | ||
| params["fps"] = fps | ||
| params["type"] = "timelapse" | ||
| else: | ||
| params["type"] = "rotating" | ||
|
|
||
| if not filename: | ||
| start_str = start.strftime("%m-%d-%Y, %H.%M.%S %Z") | ||
| end_str = end.strftime("%m-%d-%Y, %H.%M.%S %Z") | ||
| filename = f"{camera_id} {start_str} - {end_str}.mp4" | ||
|
|
||
| params["filename"] = filename | ||
|
|
||
| path = "video/prepare" | ||
|
|
||
| return await self.api_request( | ||
| path, | ||
| params=params, | ||
| raise_exception=True, | ||
| ) |
There was a problem hiding this comment.
Enhance error handling and optimize fps handling in prepare_camera_video.
- Error Handling: Add checks to ensure that
cameraandcamera.channelsare valid before accessing them to preventAttributeErrororKeyError. - Optimizing
fpsHandling: Avoid unnecessary dictionary operations by checking iffpsis notNonebefore settingparams['type']. - Test Coverage: The function lacks test coverage for several lines, as indicated by static analysis.
Here's a proposed improvement for error handling:
if validate_channel_id and self._bootstrap is not None:
try:
camera = self._bootstrap.cameras[camera_id]
if channel_index >= len(camera.channels):
raise IndexError("Channel index out of range")
except (KeyError, AttributeError, IndexError) as e:
raise BadRequest(f"Invalid input: {str(e)}") from eCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def prepare_camera_video( | |
| self, | |
| camera_id: str, | |
| start: datetime, | |
| end: datetime, | |
| channel_index: int = 0, | |
| validate_channel_id: bool = True, | |
| fps: int | None = None, | |
| filename: str | None = None, | |
| ) -> list[Any] | dict[str, Any] | None: | |
| """ | |
| Prepares MP4 video from a given camera at a specific time. | |
| This is the newer implementation of retrieving video from Unifi Protect. | |
| When using this method, it should be followed up with download_camera_video(). | |
| Start/End of video export are approximate. It may be +/- a few seconds. | |
| Providing the `fps` parameter creates a "timelapse" export wtih the given FPS | |
| value. Protect app gives the options for 60x (fps=4), 120x (fps=8), 300x | |
| (fps=20), and 600x (fps=40). | |
| You will receive a filename and an expiry time in seconds. | |
| """ | |
| if validate_channel_id and self._bootstrap is not None: | |
| try: | |
| camera = self._bootstrap.cameras[camera_id] | |
| camera.channels[channel_index] | |
| except IndexError as e: | |
| raise BadRequest from e | |
| params = { | |
| "camera": camera_id, | |
| "start": to_js_time(start), | |
| "end": to_js_time(end), | |
| } | |
| if channel_index == 3: | |
| params.update({"lens": 2}) | |
| else: | |
| params.update({"channel": channel_index}) | |
| if fps is not None and fps > 0: | |
| params["fps"] = fps | |
| params["type"] = "timelapse" | |
| else: | |
| params["type"] = "rotating" | |
| if not filename: | |
| start_str = start.strftime("%m-%d-%Y, %H.%M.%S %Z") | |
| end_str = end.strftime("%m-%d-%Y, %H.%M.%S %Z") | |
| filename = f"{camera_id} {start_str} - {end_str}.mp4" | |
| params["filename"] = filename | |
| path = "video/prepare" | |
| return await self.api_request( | |
| path, | |
| params=params, | |
| raise_exception=True, | |
| ) | |
| async def prepare_camera_video( | |
| self, | |
| camera_id: str, | |
| start: datetime, | |
| end: datetime, | |
| channel_index: int = 0, | |
| validate_channel_id: bool = True, | |
| fps: int | None = None, | |
| filename: str | None = None, | |
| ) -> list[Any] | dict[str, Any] | None: | |
| """ | |
| Prepares MP4 video from a given camera at a specific time. | |
| This is the newer implementation of retrieving video from Unifi Protect. | |
| When using this method, it should be followed up with download_camera_video(). | |
| Start/End of video export are approximate. It may be +/- a few seconds. | |
| Providing the `fps` parameter creates a "timelapse" export wtih the given FPS | |
| value. Protect app gives the options for 60x (fps=4), 120x (fps=8), 300x | |
| (fps=20), and 600x (fps=40). | |
| You will receive a filename and an expiry time in seconds. | |
| """ | |
| if validate_channel_id and self._bootstrap is not None: | |
| try: | |
| camera = self._bootstrap.cameras[camera_id] | |
| if channel_index >= len(camera.channels): | |
| raise IndexError("Channel index out of range") | |
| except (KeyError, AttributeError, IndexError) as e: | |
| raise BadRequest(f"Invalid input: {str(e)}") from e | |
| params = { | |
| "camera": camera_id, | |
| "start": to_js_time(start), | |
| "end": to_js_time(end), | |
| } | |
| if channel_index == 3: | |
| params.update({"lens": 2}) | |
| else: | |
| params.update({"channel": channel_index}) | |
| if fps is not None and fps > 0: | |
| params["fps"] = fps | |
| params["type"] = "timelapse" | |
| else: | |
| params["type"] = "rotating" | |
| if not filename: | |
| start_str = start.strftime("%m-%d-%Y, %H.%M.%S %Z") | |
| end_str = end.strftime("%m-%d-%Y, %H.%M.%S %Z") | |
| filename = f"{camera_id} {start_str} - {end_str}.mp4" | |
| params["filename"] = filename | |
| path = "video/prepare" | |
| return await self.api_request( | |
| path, | |
| params=params, | |
| raise_exception=True, | |
| ) |
Tools
GitHub Check: codecov/patch
[warning] 1575-1576: src/uiprotect/api.py#L1575-L1576
Added lines #L1575 - L1576 were not covered by tests
[warning] 1585-1585: src/uiprotect/api.py#L1585
Added line #L1585 was not covered by tests
[warning] 1590-1591: src/uiprotect/api.py#L1590-L1591
Added lines #L1590 - L1591 were not covered by tests
|
Hi, I have some time this weekend and would like to get this PR over the line and merged as it is preventing unifi-protect-backup from fully making the switch to this library without losing functionality. Could you provide details of what is needed to get this merged? |
Hi @ep1cman My outstanding review comments and the missing tests are the key blockers. The bot review comments are not generally not blocking and sometimes have some good insights |
|
Can you give me write permissions to this PR, or should I make a new one? I believe I have addressed all the comments from yourself and the bot, except one. I disagree with the bot on downloading to a temp file if one isn't provided. The code already has logic to write both to a file, or to memory. If you wish to use a temp file you can give it one. I would quite like to maintain the ability to not write files to disk as something like unifi-protect-backup can generate excessive (and unnecessary) disk IO otherwise. I just need to write tests for this. Any advice on this would be welcome. I'm unsure of how to test this functionality without access to a unifi protect instance. |
|
You should have write access now |
|
|
Adjusted a few more settings, please try again |
Sadly still no joy. I've had this work as a repo owner before but I dont think ive ever tried editing someone elses PR as a non-owner. Will it just be easier to open a new PR? |
Yeah I think thats the way to go |
Description of change
Pull-Request Checklist
mainbranchFixes #0000(N/A) (export fails frequently when used on new versions of Protect)Summary by CodeRabbit
New Features
download_camera_videomethod to download prepared MP4 videos from cameras.prepare_camera_videomethod to prepare MP4 videos from cameras at specific times.Improvements
export_camera_videomethod to include new parameters for better functionality.Deprecations
get_camera_videomethod. Users are encouraged to useprepare_camera_videoanddownload_camera_videofor compatibility with Unifi Protect 4 or later.Chores
.gitignoreto include.vscode/and.idea/directories.