rsjudka / intelligent-auto

41 stars 15 forks source link

Camera #36

Closed stefan-sherwood closed 4 years ago

stefan-sherwood commented 4 years ago

Camera tab - camera connected image

Camera tab - camera disconnected (needs work) image

Settings tab - camera connected image

Settings tab - camera disconnected image

rsjudka commented 4 years ago

What do you think about using this icon instead? https://material.io/resources/icons/?search=camera&icon=camera_alt&style=baseline

i would prefer it because its simpler and feels more generic (say in the future we allow the tab to capture video stills or something like that)

stefan-sherwood commented 4 years ago

What do you think about using this icon instead? https://material.io/resources/icons/?search=camera&icon=camera_alt&style=baseline

i would prefer it because its simpler and feels more generic (say in the future we allow the tab to capture video stills or something like that)

One is a video camera icon and the other is a still camera icon. It doesn't seem more suitable to me since the primary function of the tab is to show a live video camera view but I don't feel strongly about it.

rsjudka commented 4 years ago

all files should end in a newline

rsjudka commented 4 years ago

so the text input fields...

if youve noticed, none of ia currently has any sort of text input because you cant guarantee the user has a keyboard (physical or virtual). Thats why for the wireless connection i did that "tap to set each number"

definitely something that's going to need to be rethought and figure out how to set these without a keyboard

rsjudka commented 4 years ago

Now im wondering if we even need an independent settings tab for the camera..

What if we just do something similar to the wireless connection or app launcher, where we just allow the user to set the appropriate values directly in that tab

this makes sense to me since a user really shouldn't be able to change settings while the camera is active

stefan-sherwood commented 4 years ago

I have implemented several more features for the camera that require settings. I agree that having access to settings from each tab is useful but we're talking about mixing to different approaches.

image

rsjudka commented 4 years ago

not really sure why those cant just be set in the camera tab tho

image

you can basically have a stacked widget that once the camera feed connects it takes over the tab (and hides the settings)

these settings seem more tied to a connected feed instead of something that needs to be manipulated globally

stefan-sherwood commented 4 years ago

The camera popup is something that is independent of the camera tab. The idea is that if you put the car in reverse, you can see what's behind you from whatever tab you're on.

image

stefan-sherwood commented 4 years ago

I also have an on-screen keyboard implemented but not yet ready for PR.

rsjudka commented 4 years ago

The camera popup is something that is independent of the camera tab. The idea is that if you put the car in reverse, you can see what's behind you from whatever tab you're on.

so what happens if the popup is opened and switch over to the camera tab? Most "video in window" features I've seen still seem to have the popup and original screen linked, where either you have the popup or screen, but not both

rsjudka commented 4 years ago

I also have an on-screen keyboard implemented but not yet ready for PR.

do we want an onscreen keyboard tho? that can take up a lot of space and may even cover up the field you're trying to edit. Is there no other way to define what camera you want to use without text input?

stefan-sherwood commented 4 years ago

do we want an onscreen keyboard tho? that can take up a lot of space and may even cover up the field you're trying to edit. Is there no other way to define what camera you want to use without text input?

I don't know. I need to try it out and get answers to your questions. The other option is speech recognition, about which I know quite a bit since I was one of the inventors of the tech :)

The camera name is unimportant unless you have multiple cameras. And the stream URL you will generally only set up once.

stefan-sherwood commented 4 years ago

so what happens if the popup is opened and switch over to the camera tab? Most "video in window" features I've seen still seem to have the popup and original screen linked, where either you have the popup or screen, but not both

That's what I was thinking. Right now it shows both, which is ok if they are different streams and just weird if they're the same. That's of of the last two major parts of the popup feature I haven't made yet. I will also let you choose a trigger, which for most people will be via one of the hardware pins connected to either the reverse on the transmission or a button.

stefan-sherwood commented 4 years ago

inline QString get_brightness_module() { return this->brightness_module; }

I see a lot of this in config.hpp. Is there a case for using the optional keyword inline and the superfluous this? I would write it as:

QString get_brightness_module() { return brightness_module; }

rsjudka commented 4 years ago

inline QString get_brightness_module() { return this->brightness_module; }

I see a lot of this in config.hpp. Is there a case for using the optional keyword inline and the superfluous this? I would write it as:

QString get_brightness_module() { return brightness_module; }

inline is just an optimization thing and I prefer to use this to denote any class variables/functions

rsjudka commented 4 years ago

The camera name is unimportant unless you have multiple cameras. And the stream URL you will generally only set up once.

hmm I would think we would want to support only one camera at a time, would there be a reason to allow more than one?

stefan-sherwood commented 4 years ago

inline is just an optimization thing and I prefer to use this to denote any class variables/functions

I believe that in release builds, every compiler will automatically inline low-complexity functions and none will inline non-low-complexity functions, even if they are marked inline.

this versus non-this is just a style preference so I will conform.

stefan-sherwood commented 4 years ago

hmm I would think we would want to support only one camera at a time, would there be a reason to allow more than one?

Obvious uses of cameras are:

A lot of people like to have dashcams in case there's an accident they can have a recording of what really happened.

Obvs we don't have to support all of this but there certainly is a use case for it.

rsjudka commented 4 years ago

A lot of people like to have dashcams in case there's an accident they can have a recording of what really happened.

I guess i never thought of it for recording, just more of a live video feed. Sounds like with all this we can make an open-source tesla :grinning:

But I can see a lot of complexity (and power consumption) coming from supporting multiple cameras. Would you limit the number of cameras? Limit the number of popups? How would you prevent app slowdown with multiple cameras?

rsjudka commented 4 years ago

I believe that in release builds, every compiler will automatically inline low-complexity functions and none will inline non-low-complexity functions, even if they are marked inline.

Well inline isn't really for the compiler (it is a hint but the compiler is usually smarter) but more for the linker.

I will admit i did get a little inline happy, but for getters/setters and functions defined in the header (which should all be short and simple) i see the benefit of it (and like it from a stylistic standpoint :p)

rsjudka commented 4 years ago

about which I know quite a bit since I was one of the inventors of the tech :)

nice flex :wink: not going to lie i did a little creeping and youve done some impressive work! glad you found your way here :slightly_smiling_face:

stefan-sherwood commented 4 years ago

I will admit i did get a little inline happy, but for getters/setters and functions defined in the header (which should all be short and simple) i see the benefit of it (and like it from a stylistic standpoint :p)

Not sure if I was clear. I was suggesting leaving them as inline in the header but just removing the keyword.

stefan-sherwood commented 4 years ago

I guess i never thought of it for recording, just more of a live video feed. Sounds like with all this we can make an open-source tesla 😀

I think this will be standard in cars at some point. Insurance companies should offer a discount for it but none do yet. There's actually a bill in NY to mandate a discount for people with them.

But I can see a lot of complexity (and power consumption) coming from supporting multiple cameras. Would you limit the number of cameras? Limit the number of popups? How would you prevent app slowdown with multiple cameras?

I agree that resource consumption may be an issue. I have already implemented on my machine a disconnection from the stream when you aren't actually viewing it. If you're only viewing one stream at a time it doesn't matter how many cameras you have connected. For recording I wouldn't do any decoding or rendering so the CPU overhead should be minimal.

The bigger concern would be that SD card tech isn't well suited to continuous recording as it has a limited number of writes. I suspect they use a circular memory buffer and only write to disk when you tell it that something interesting just happened. That would require a lot of memory though.

I have multiple cameras running on my machine and it works fine but it's a VM on a powerful desktop machine so it doesn't "count". I will have to do testing on the target hardware. That leads me to the question: where are we with Rpi4 support? That would be my preferred target since it is much faster than even the 3B+

stefan-sherwood commented 4 years ago

nice flex 😉 not going to lie i did a little creeping and youve done some impressive work! glad you found your way here 🙂

Thanks! I was planning to do all of this myself so I am very happy that there are other people dedicated to making it happen.

rsjudka commented 4 years ago

That leads me to the question: where are we with Rpi4 support? That would be my preferred target since it is much faster than even the 3B+

I do all my testing on a pi4, and it works great!

stefan-sherwood commented 4 years ago

Updated screenshots:

Default start state: image

After pressing "connect" button: image

Connection failure: image

Connected (connection status in lower-left, disconnect button in lower-right): image

stefan-sherwood commented 4 years ago

I think this is ready to be merged, pending final review. The only missing feature is auto-connect on startup, which is easy to implement but not in MVP.

stefan-sherwood commented 4 years ago

Updated camera icon

image

rhysmorgan134 commented 4 years ago

Just reading through this thread, looks like it's going to be a pretty cool feature. I can see the use for multiple cams for sure when it comes to off road vehicles.

For the recording part of it, I have a dash cam set up that does front and rear, it's a Chinese make, but they have rtsp streams, but they also have an API where you can stream the recordings, or browse the files, download etc.

Might be a nice way to implement it without adversely affecting performance.

rsjudka commented 4 years ago

looking good! (and thank you for all the work you've put into this)

Just some general notes:

and then some thoughts on the design:

rsjudka commented 4 years ago

will this work with usb-cams too? the text and icon seem to suggest its a wireless thing

stefan-sherwood commented 4 years ago

Just some general notes:

  • can you use this anywhere you access class variables/functions? Just helps me follow along with whats going on and my stylistic preference

I am trying to conform, even in cases where I disagree with the effect on readability.

  • remove all the unused files (and update the qrc file)

Some vc systems mishandle files that are deleted and later put back and I think these files will soon be reintroduced. But I will remove them and deal with it later since that's your preference.

  • does the status need to be displayed? I would guess its pretty obvious when the camera is connected, but when the camera gets disconnected wouldn't it be better to just switch to the connect dialogue?

The behavior of video streams with different source and destination machines is yet untested and the idea is that this could give more insight.

  • maybe instead of a disconnect button, you can just have an "x" at the top right corner to close the connection? To kinda match the UX of the app launcher

I am all for consistency but I don't see an x at the top right of the app launcher. I also don't understand the app launcher UI in general so it's hard for me to make sure like items follow the same patterns.

Here is what I see: image

  • can you style the text input? (follow the material design where it makes sense)

Does Qt provide a mechanism for the embedding of the label during input? I don't see it. I can certainly change the colors.

image

stefan-sherwood commented 4 years ago

We really should consider using stylesheets so that developers can just add controls and have them automatically styled according to the theme. But that is not in the scope of this pull request.

Edit: oops, just saw the stylesheets. nvm

rsjudka commented 4 years ago

I am all for consistency but I don't see an x at the top right of the app launcher. I also don't understand the app launcher UI in general so it's hard for me to make sure like items follow the same patterns.

Yeah sorry its more of an "experimental design" currently, but basically the left box has the directories and the right box has any executables in the current directory.

The "X" will show up (at the top right) if you launched an app, for example: image

rsjudka commented 4 years ago

Does Qt provide a mechanism for the embedding of the label during input? I don't see it. I can certainly change the colors.

I dont think Qt does. I would try just styling the basics of it (so background color and bottom border) and also dont worry about the different states

stefan-sherwood commented 4 years ago
  • can you use this anywhere you access class variables/functions? Just helps me follow along with whats going on and my stylistic preference

I'm going through and adding a ton of this-> prefixes. As a side note, this is why some people use Hungarian notation because it's less clutter to have a m or m_ prefix to member variables than this->.

stefan-sherwood commented 4 years ago

Screenshots with edit control styling:

Dark (no focus): image

Dark (with focus): image

Light (no focus): image

Light (with focus): image

rsjudka commented 4 years ago

just in case you missed this...

will this work with usb-cams too? the text and icon seem to suggest its a wireless thing

stefan-sherwood commented 4 years ago

will this work with usb-cams too? the text and icon seem to suggest its a wireless thing

This is presently a network streaming camera only thing. That doesn't exclude local cameras but they will have to present a network stream interface. Proper support of local cameras will be added in subsequent feature enhancements, probably starting with PiCam but open to discussion.

stefan-sherwood commented 4 years ago

will this work with usb-cams too? the text and icon seem to suggest its a wireless thing

Talk to @rsjudka about the icon :)

stefan-sherwood commented 4 years ago

will this work with usb-cams too? the text and icon seem to suggest its a wireless thing

Talk to @rsjudka about the icon :)

Hahaha just realized that was your comment. This is why I put in a generic "video" icon in the first place. No worries it can always be changed.

rsjudka commented 4 years ago

Hahaha just realized that was your comment. This is why I put in a generic "video" icon in the first place. No worries it can always be changed.

though you were just being snarky :rofl:

i still prefer the camera icon, but this one has the little wifi logo thing at the top so i was just curious (since you said it'll only work with network streams, i would say leave it)