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

Add Digital PTZ via mouse and touch #491

Merged

Conversation

dbuezas
Copy link
Contributor

@dbuezas dbuezas commented May 21, 2023

Adds configurable Digital (local, fake, css-based) zoom and panning via mouse and touch.
The viewport setup can be persisted so it survives page reloads, and seamlessly adapts between landscape, portrait and full screen modes.

The navigation gestures are the same as in google maps and can be disabled one by one.

For you to consider:

  • I enabled all gestures by default.
  • This may make scrolling difficult for people that has multiple videos in the same panel.
  • It may be more reasonable to enable only these by default
digital_ptz:
  mouse_drag_pan: true
  mouse_wheel_zoom: true
  mouse_double_click_zoom: true

  touch_pinch_zoom: true
  touch_drag_pan: true
  touch_tap_drag_zoom: true

  persist: true
  • Persistence is enabled by default
  • I am not sure if there is any setting fro WebRTC with which all of this would be incompatible. Please let me know.
  • It ended up being a fair bit of code, partially because I made extra effort in dividing into multiple classes for easier maintenance.
  • To avoid silly mistakes, I actually made this in typescript and transpiled it back to js.

I'm quite happy with how it feels and find it quite useful myself. I'm looking forward to your feedback :)

@dbuezas dbuezas marked this pull request as draft May 22, 2023 07:31
@dbuezas
Copy link
Contributor Author

dbuezas commented May 22, 2023

Some combinations of options behave unexpectedly. I'll make a second pass tonight.

@dbuezas dbuezas marked this pull request as ready for review May 22, 2023 21:14
@dbuezas
Copy link
Contributor Author

dbuezas commented May 23, 2023

I've been testing it for a couple of days and have polished a couple of rough edges.

@dbuezas
Copy link
Contributor Author

dbuezas commented May 24, 2023

I think I'm satisfied now. I made a (hopefully) last round of cleanup, simplifications and optimisations.
Looking forward to your feedback, I'm using it quite a bit myself and it feels very natural to me. I think if feels as if it was natively supported by the video tag.

@zelo66
Copy link

zelo66 commented May 31, 2023

Great work,

1.I've been testing this for a week and it works great on my phone and in regular preview on my computer. Unfortunately in full screen mode on PC it doesn't work for me. I tested on two computers, in chrome and opera.

My settings:

digital_ptz:
  mouse_drag_pan: true
  mouse_wheel_zoom: true
  mouse_double_click_zoom: true
  touch_pinch_zoom: true
  touch_drag_pan: false
  touch_tap_drag_zoom: true
  persist: false
  1. can I use the buttons in the UI to zoom in and out?
    Such as you have given in your example with lovelace-card-mod

  2. can I add a button that will point and zoom in the right way to the desired location? E.g. I will set a gate position and when I press the gate button the card will automatically zoom in and move to the selected location?

@dbuezas
Copy link
Contributor Author

dbuezas commented Jun 1, 2023

Hey @zelo66,
thanks for the feedback.

  1. For it to work in full screen in desktop, you need to enable webrtc's custom ui
ui: true
  1. There's no support for zooming with buttons at the moment. I could add it as a new feature after this PR is merged (if Alexey agrees)
  2. Also no feature available to set and activate presets. At some point I want to add an option to draw a box to zoom there. If/when I do, I can add some connection to trigger that via buttons.

@dbuezas
Copy link
Contributor Author

dbuezas commented Jun 3, 2023

Hey @AlexxIT I assume you just haven't had time yet, but let me know if there is something about the code you'd like to be implemented differently (e.g changing defaults, explaining something with comments, ...)

I understand that it is not a very small PR, I just can't find a way to make this more compact without compromising readability or corner cases (like handling full screen video).

@zelo66
Copy link

zelo66 commented Jun 4, 2023

Hey @zelo66, thanks for the feedback.

  1. For it to work in full screen in desktop, you need to enable webrtc's custom ui
ui: true
  1. There's no support for zooming with buttons at the moment. I could add it as a new feature after this PR is merged (if Alexey agrees)
  2. Also no feature available to set and activate presets. At some point I want to add an option to draw a box to zoom there. If/when I do, I can add some connection to trigger that via buttons.

Of course, I have set
ui:true

By it not working I meant that the image zooms in just a little. I have recorded a gif. On my main camera the image zooms in even less (there is a zoom effect which disappears after about 0.5 seconds.

I checked chrome, opera and firefox.

In Chrome and Opera zooming in full screen has the effect as in the gif. In Firefox the zoom works very well, but the " mouse_double_click_zoom" completely does not work.

222 (1)

@dbuezas
Copy link
Contributor Author

dbuezas commented Jun 4, 2023

Do you have any custom styles by any chance?

@zelo66
Copy link

zelo66 commented Jun 4, 2023

No, everything is in the default settings.

I have now checked in one more instance of HA, plus on another computer. Same effect. It zooms in a bit, looks like the aspect ratio changes and blocks further zooming.

@dbuezas
Copy link
Contributor Author

dbuezas commented Jun 4, 2023

I can't reproduce this so I am not sure what could be happening. Please share the yaml of the card and go2rtc stream you are using for the example. Also version of HA and OS of the pc you recorded the gif with.
Also please make sure you are running the latest commit of this PR

@zelo66
Copy link

zelo66 commented Jun 8, 2023

type: custom:webrtc-camera
url: rtsp://zephyr.rtsp.stream/movie?streamKey=19b175b07ab0c45c0d22c3fa68797415
poster: https://home-assistant.io/images/cast/splash.png
muted: true
ui: true
webrtc: true
digital_ptz:
  mouse_drag_pan: true
  mouse_wheel_zoom: true
  mouse_double_click_zoom: true
  touch_pinch_zoom: true
  touch_drag_pan: false
  touch_tap_drag_zoom: true
  persist: false

For this example, I am using a test stream from the website:
https://rtsp.stream/

HA:
Home Assistant 2023.5.4 Supervisor 2023.06.1 Operating System 10.2 Interfejs użytkownika: 20230503.3 - latest

OS:
Win 10 Home 22H2

Of course I use the latest commit. I've also found that when I turn on the developer options the zoom works normally in full screen mode. (As long as developer mode is open).
NOWY4 (1)

@dbuezas
Copy link
Contributor Author

dbuezas commented Jun 9, 2023

It's probably windows related. I don't have a windows machine to debug it. Do you think it is a blocker for merging?

@AlexxIT
Copy link
Owner

AlexxIT commented Jun 16, 2023

Hi @dbuezas. Just had time to look at your work. Two things bother me:

  1. A lot of extra external files for just one feature that only a part of users will use. Each will be downloaded to the client individually. Not the best approach in modern web development. Perhaps one file would be enough. Or even including all the code in webrtc-camera.js. Because video-rtc.js is the streaming core from go2rtc and is copied "as is". And webrtc-camera.js has all the extra functionality.
  2. I'm not sure that all constructions will work in all browsers. Especially in horrible iOS older versions. Users of old iOS are also human beings and deserve to have normal streaming cameras. Of course, as far as Apple has allowed it. I'm not sure about constructions like ...

@dbuezas
Copy link
Contributor Author

dbuezas commented Jun 16, 2023

Hi @AlexxIT,

1. Multiple files

I think the browser will reuse the same http connection to grab all files efficiently. I read it is only discouraged if the dependency tree is very deep, and I am afraid my code will make the webrtc-camera.js too big to work comfortably with. But anyway, your repo your rules :)

What do you prefer?

  • One dedicated file with all the functionality
  • All inside the webrtc-camera.js file
  • A small and simple bundling script so you can modularise everything but serve a single file (if you want i can use parceljs which requires no extra config files)

2. Modern javascript syntax

I checked the browser versions required for each modern operator:

So it looks like we are safe. Again, your repo, so if you just don't like a particular operator I can remove it.


Let me know and I'll apply your request on the weekend.
If you don't actually like the feature yourself, I can of course just make a fork for the screen pinchers like me, I'll understand that too.

@AlexxIT
Copy link
Owner

AlexxIT commented Jun 17, 2023

  1. I think one extra file can have all zoom functionality
  2. Looks like my fears were wrong

@dbuezas dbuezas force-pushed the Add-digital-ptz-mouse-and-touch-control branch from 49cac43 to d0829c0 Compare June 18, 2023 09:10
Also, don't prevent scrolling on touch drag unless zoom scale != 1
@dbuezas dbuezas force-pushed the Add-digital-ptz-mouse-and-touch-control branch from d0829c0 to 1e15ec0 Compare June 18, 2023 09:41
@dbuezas
Copy link
Contributor Author

dbuezas commented Jun 18, 2023

Hey @AlexxIT,
as requested, all the code is now in a single file digital-ptz.js.
I also:

  • Rebased so it can be merged.
  • Fixed interference between touch panning and scrolling in mobile (scroll unless zoomed), which now allows for enabling it by default
  • Added a way to disable digital ptz completely via digital_ptz: false

Let me know if you find something else you'd like to be done differently. I obviously like the feature so I enabled it by default, but you may prefer it to be disabled by default instead and require writing digital_ptz: to enable it

@dbuezas
Copy link
Contributor Author

dbuezas commented Jul 6, 2023

Hey Alex!
Does anything else need clarification or improvement?

@AlexxIT
Copy link
Owner

AlexxIT commented Jul 6, 2023

No. I'm planning to include this in next release. Just needs some time.

@AlexxIT
Copy link
Owner

AlexxIT commented Jul 6, 2023

Just as I thought - the iOS12 test did not pass.

I know it's a very old browser. But Hass somehow works in it. And WebRTC somehow works in it.

And with these innovations, the whole WebRTC card is falling down :(

PS. What you think about external "HACS Frontend plugin" for WebRTC card? Maybe it's possible to add plugins functionality to WebRTC card.

@dbuezas
Copy link
Contributor Author

dbuezas commented Jul 6, 2023

Oh, no!
Do you know exactly what failed? I can try to fix it quickly

@AlexxIT
Copy link
Owner

AlexxIT commented Jul 6, 2023

I have left many comments to code

@dbuezas
Copy link
Contributor Author

dbuezas commented Jul 6, 2023

I don't see them yet. Maybe is the review not marked as finished?

custom_components/webrtc/www/digital-ptz.js Outdated Show resolved Hide resolved
custom_components/webrtc/www/digital-ptz.js Show resolved Hide resolved
custom_components/webrtc/www/digital-ptz.js Outdated Show resolved Hide resolved
custom_components/webrtc/www/digital-ptz.js Outdated Show resolved Hide resolved
@dbuezas
Copy link
Contributor Author

dbuezas commented Jul 6, 2023

Ok, it should work now.
I ensured the code is compatibly by finding out what ECMAScript version is compatible with iOS12 (it is ES2015) and setting the typescript transpilation target to it.

Sorry for that

@dbuezas dbuezas force-pushed the Add-digital-ptz-mouse-and-touch-control branch from 503641b to ed5450c Compare July 6, 2023 21:49
@dbuezas
Copy link
Contributor Author

dbuezas commented Jul 9, 2023

I applied all fixes and moved initialisation to its own member function so it is consistent with the rest of your code.

@AlexxIT
Copy link
Owner

AlexxIT commented Jul 10, 2023

Looks like all works fine now. But pinch action enables full screen on iPhone. But it is not your problem :)

@AlexxIT
Copy link
Owner

AlexxIT commented Jul 10, 2023

Thanks to you, I have researched ESLinter and will add it to the next release. It will help to avoid such compatibility problems.

@dbuezas
Copy link
Contributor Author

dbuezas commented Jul 10, 2023

That's a very good idea!
And prettier is great for code formatting. If you haven't tried typescript yet, I think you'd like it too :)

@AlexxIT
Copy link
Owner

AlexxIT commented Jul 10, 2023

I don't want to implement any JS build stage to my projects yet. It will unnecessary increase complexity.

PS. I'm using prettier from PyCharm.

@dbuezas
Copy link
Contributor Author

dbuezas commented Jul 10, 2023

True, it does add a step. I mostly work with typescript so I'm very used to it.
For digital-ptz.js I used prettier locally, let me know if I should use a different format

@AlexxIT AlexxIT merged commit ee3600f into AlexxIT:master Jul 11, 2023
@dbuezas
Copy link
Contributor Author

dbuezas commented Jul 13, 2023

I didn't consider people using style transforms themselves. I'll try adding a wrapper element around the video and transform that one instead this way they should be able to coexist.

@AlexxIT
Copy link
Owner

AlexxIT commented Jul 13, 2023

They use it only for rotation and changing aspect ratio

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

Successfully merging this pull request may close these issues.

3 participants