Feature hot clip

[bkleiner]

• adds function to control dvr directly
• adds right button handlers to pages
• reworks playback sdcard scan
• marks clip as "hot" on right button

[evilC]

Picking up discussion from #96 ...

I am no C expert, and unfamiliar with the code base, but what happens? is hdz_123 renamed to hot_hdz_123 or just hot_123? If the former, what happens if you tag the same file twice? Does it become hot_hot_hdz_123?

let's discuss this in the PR

Furthermore - with this feature being implemented, I would say that #97 becomes a little more complicated - I think we should have a "Delete all recordings except hot recordings" option.

i don't see really see how it nessacitates that, but such a function can be considered.

I would not say that it nessacitates it, but if you have a bunch of hot recordings on your SD card, and it becomes full, currently there is no way to free up space whilst preserving the hot clips (Without removing the SD card, putting it in another device)

Also, wrt #98 , this current implementation maybe precludes that? What's the plan here? If that is to be implemented, would the "Mark as hot" option become an entry in the context menu?

Why would it preclude that? really that PR offers a basis for further extension. Previously the right button could only be used while on the video screen. Yes, the mark as hot can be moved to the context menu if that menu were to be implement.

Yeah, preclude is maybe a poor choice of words there - all I meant was that if the right button was now used to mark hot, it could not be used to open the context menu - as you say, it can be made an option.

[evilC]

Also, just to add - I feel that maybe #98 should be considered before making a new release, as this PR as it stands would require an update to the manual (ie, the feature is non-discoverable? I don't see any evidence of something which informs the user that this option exists, although admittedly I have not tested it yet, just had a quick glance through the code)
Even if this is the only option present in the context menu for now, it may be worth implementing the context menu so that the manual could be updated with the fact that the context menu exists and not require multiple re-releases of the manual.
At least, of course, the functionality to open a context menu whilst in the menu system - the request in #98 for opening a context menu whilst in the camera feed is maybe a little more involved and could be pushed to a future release or maybe not even implemented at all.

[bkleiner]

Also, just to add - I feel that maybe #98 should be considered before making a new release, as this PR as it stands would require an update to the manual (ie, the feature is non-discoverable? I don't see any evidence of something which informs the user that this option exists, although admittedly I have not tested it yet, just had a quick glance through the code) Even if this is the only option present in the context menu for now, it may be worth implementing the context menu so that the manual could be updated with the fact that the context menu exists and not require multiple re-releases of the manual. At least, of course, the functionality to open a context menu whilst in the menu system - the request in #98 for opening a context menu whilst in the camera feed is maybe a little more involved and could be pushed to a future release or maybe not even implemented at all.

I am sorry, but what exactly do you expect me to do here?

Hold back this PR until somebody comes around and implements a context menu on top of it?
The context menu is not trivial (on either screen) and will require a refactor of the whole input system. doing that in one swoop is gonna be error prone and by the time its ready this PR will be outdated to a point where you can basically redo it.

Merge the PR and tell divimath that they can not create a new release until somebody comes around and implements the context menu?

To me it feels like just not having this feature in the manual is way more acceptable than any other option, seeing that this is an additional feature and does not remove any functionality, i barely see any harm.
I do agree that manual will need updating eventually (elrs, ht and the like are not included either) but stopping the whole release machinery because of it seems woefully overkill to me.

For the time being we can simply add a text explaining the feature to the page.

[evilC]

Apologies, I was not aware that implementing a context menu (Certainly in the menu) would be that big of a job. Makes much more sense to just merge it in for now and worry about that later then.

[evilC]

@bkleiner I just tried the latest build and it appears to have a bug.
The first time you tag a clip, it renames it hot_hdz_001 just fine, but the 2nd time you tag a clip, the existing hot_hdz_001 disappears, and the new one replaces it

[bkleiner]

@bkleiner I just tried the latest build and it appears to have a bug. The first time you tag a clip, it renames it hot_hdz_001 just fine, but the 2nd time you tag a clip, the existing hot_hdz_001 disappears, and the new one replaces it

yepp, you are right, i was trying to be smart and made a last minute off-by-one error.
pushed a potential fix, cant test myself right now tho.

[evilC]

yep, seemed to work.
Interestingly tho, I had hot file 1 already present, scrolled right to next clip, tagged it, it was correctly named hot 2, but it got put at the end of the list. I scrolled back to start, tagged the next clip, and it renamed it and kept it at the beginning. Did the same a 2nd time, and again good.

I just rebooted the goggles afterwards tho (To check if the clip that got put at the end got put back to the start), and now I cannot get into the playback menu. I can get into other menus just fine Ignore. Like a muppet I had ejected the SD card

[evilC]

But still the oddness with hot clip 2 at the end.
ie the order of the list in the goggles is hot_1, hot_3, hot_4, hot_5, (loads of untagged files), hot_2

[bkleiner]

humm, the code does not do any ordering itself, it relies on the kernel to return the dir entries in order.
i gonna have to check on the details here and implement ordering on our end if all else fails.

[pkendall64]

The ordering is an artefact of the way that mv works internally, by using link/unlink.
If you want the list ordered, which I think we do, then we might be better off using scandir rather than opendir.

[bkleiner]

The ordering is an artefact of the way that mv works internally, by using link/unlink. If you want the list ordered, which I think we do, then we might be better off using scandir rather than opendir.

lovely! pushed a version using scandir.

[evilC]

Confirmed as fixed.
Although there is now another slight issue - if I tag, for example, the 5th clip, it renames it correctly (And the clip moves to become the first in the list), and the "currently selected" red border stays where it is (Highlighting the 5th clip in the list), but when you then scroll to the right, the highlight moves back to the first clip in the list.
So basically, when you mark a clip as hot, the "currently selected" box needs to be changed to the clip that was after the clip that you just tagged.
Not a huge issue though, it's usable. I am flying tomorrow, so I will get to actually fly this build and see how it goes.

[evilC]

I flew this build today, and very bad things happened :(
I ended up losing a quad (My own fault, flew it with a known dodgy XT30 connector, a known defective lost model alarm, and beta goggle firmware) - during the flight I got massive interference and went down, got a battery disconnection and the defective lost model alarm did not go off - so no fault with the video feed I would say, but the DVR is all messed up, so I was not sure where the quad was (Certainly in the field - looking back now I think I maybe know where it is)

This happened on the very last flight of the day - here is a screenshot of how my DVR folder looked:

Some notes:

1. The folder was empty at the start of the day - so all weirdness seen in this folder is from usage today, with this build.
2. The DVR was ALWAYS set to TS, so there should be no MP4s in there
3. hdz_013.mp4 is unviewable and no tools I try can find any video inside it. I am pretty sure that file is the original DVR for when I lost my quad, before it got made hot and moved to hot_hdz_007.mp4
4. The final flight of the day actually appears to be hot_hdz_007.mp4 although it is heavily corrupted. VLC can view it, but there is really weird stuff going on in the file - two images from two different points in time on the same frame. I have uploaded it here. Out of interest, the point I lost video is 6:15 - thank GOD for the record audio function, I can hear myself say that I have lost video, and can see breakup in the video at that point, so I have a fairly high confidence that this video is that last flight where I lost my quad.
5. I never marked hot_hdz_007.mp4 clip as hot - from the moment I lost video to the end of the day I was looking for the quad or trying to review DVR (But looking at the wrong file, cos hdz_011.ts is the last viewable file) so I am 99% sure that I would not have marked that video as hot.
6. There are garbled filename files / folders in there, which does is a bad sign.
7. There's an hdz_009.jpg, but no corresponding .mp4 or .ts
8. In a bunch of the files (including the hot_hdz_007.mp4` I uploaded, the DVR keeps recording when I enter the menu. You see a zoomed in, cropped, often glitchy view of the menus, and the audio keeps recording too. In fact, in one of them, I entered the DVR menu and played back a file - you can see me reviewing the DVR - in this case the video is of me reviewing the DVR, and the audio is from when I am watching it in the DVR (ie you can hear me commentating about what I can see in the DVR)
9. On coming back home and troubleshooting, I have noticed that the manual record seems to be somewhat broken. I used a fresh SD card and set the goggles to MP4 recording and manual (One of the recovery apps I was trying wanted a sample of a known good file) and I could start using the record button, but pressing the record button again did not stop. This then corrupted the MP4, so I could not use it as a known good file. Tools I have used before to fix truncated MP4s were unable to fix them.
10. hot_hdz_002.ts starts mid-way through a flight. It's clear that hdz_005.ts is the moments before file hot 2 - from the audio recording you can hear me mid-sentence at the end of hdz_005.ts and the sentence continues in hot_hdz_002.ts. File 5 is 725MB in length - it shouldn't be stopping there and starting a new file surely? I am guessing that when I finished that flight, I marked hdz_005.ts as hot and it created hot_hdz_002.ts, but not all of the recording made it into that file?
11. On getting to the end of a pack and unplugging, the video cuts out but the DVR continues recording for a while and contains just audio
12. At some point in file hdz_011.ts see this:
    
    Notice the lack of thumbnail for file hot_hdz_005 (Can't tell if it's an MP4 or TS) - however in the folder at the end of the day, there is a thumbnail for hot_hdz_005.ts

So yeah, whole loads of bork going on here.
Now I am not 100% sure if all of this is to do with the new code - this is only the 2nd time I have flown the goggles, and I did not review the DVR from the first time, but certainly some of the issues seem to be related to hot clips.

[bkleiner]

i'm sorry you had a bad experience, but i don't know how much of that is related to the subject of this PR.
The recording itself takes place in a completely separate process, so i don't think anything relating to that was caused by the changes here.
Starting and stopping of the record in that separate process is however modified by this PR, so we gonna have to take a closer look there.
Garbled file names i have a hard time seeing being caused by anything relating to either process. Maybe your filesystem got corrupted somehow?

[evilC]

I just saw something in a video which maybe yields a clue... I just saw a DVR of me going into the playback menu and marking that "last flight" as hot (Thinking that recording had ended when I unplugged the quad). ie the DVR is still recording a clip while I mark that very same clip as hot.
Surely it moving a file while it is in use would be potentially very bad?
[Edit] So I guess the problem is two-fold:

1. It keeps recording after you unplug
2. It's then possible to go into the playback menu while it is still recording

[bkleiner]

Surely it moving a file while it is in use would be potentially very bad?

no optimal for sure, but linux should handle this gracefully for the most part. it will hold on the the inode, so as long as the recording has the file open it should continue to "just work". (...otherwise the recording would stop dead there.)

1. It keeps recording after you unplug
2. It's then possible to go into the playback menu while it is still recording

yes, i think that's the problem we are looking at here: the record does not get terminated when it is supposed to.

[evilC]

Another observation is that the OSD persists in the goggles once you unplug. Maybe somehow this is keeping it recording? Or could it be that the audio is still recording that's keeping it "alive"?

[bkleiner]

Another observation is that the OSD persists in the goggles once you unplug. Maybe somehow this is keeping it recording? Or could it be that the audio is still recording that's keeping it "alive"?

no. osd data being displayed or not does not relate to the connected state.

i have pushed a potential fix, the recording would simply not get stopped properly, because there were some missing break statements. additionally i have tried to improve thread safety, as the function would release the mutex before recording actually stopped.

[AlexandreBonneau]

Just a quick UX suggestion about the naming of hot clips.
Right now since the RTC isn't powered, only the DVR filenames approximately gives an idea of the files you recorded.
Hence I surmise it's really important to keep the files in the order there were recorded.

To that end you cannot name the hot files in a way that interfere with that count order.
Instead of doing:
(the 'real' order in which the files were recorded irl are in parenthesis)

hdz_001.ts (1)
hdz_003.ts (3)
hdz_005.ts (5)
hot_hdz_001.ts (2)
hot_hdz_002.ts (4)

you could just add _hot at the end of the usual filename, like:

hdz_001.ts (1)
hdz_002_hot.ts (2)
hdz_003.ts (3)
hdz_004_hot.ts (4)
hdz_005.ts (5)

This way you still get the only clue about when a dvr was recorded, which is the order of the files between themselves.

Bonus: Also, while we are modifying how the DVR file are named, perhaps it could you useful to use 4 numbers instead of 3, like in the VRX. Since you can use very big sd card, you could theoretically go past 1000 recordings.

[evilC]

It depends on how the DVR names files - as long as we could avoid a scenraio like this, then I think this sounds like a reasonable idea:

1. Do a pack, DVR is hdz_005
2. Mark hot, file becomes hdz_005_hot
3. Power down goggles
4. Power up and fly next pack
5. DVR creates another hdz_005

[bkleiner]

Again, we simply do not know how the record app generates it names, so i went for a variant that makes collisions very unlikely.

[timfredriksson]

The firmware works good for me without any problems. Simple and straight forward to use

[evilC]

Had another day's flying - all the problems seem gone with this latest build.
And as an added bonus, we found my lost quad :)
Thanks @bkleiner for a great feature!

[evilC]

Actually, would it be possible to mark a clip hot without having to go into the menu,
If you are on the camera view page, and you have no current connection, it would be nice to be able to hit record, and it marks the last clip (highest number) as hot. Ideally it should show some prompt like "Marked hdz_009 as hot"

[bkleiner]

Actually, would it be possible to mark a clip hot without having to go into the menu, If you are on the camera view page, and you have no current connection, it would be nice to be able to hit record, and it marks the last clip (highest number) as hot. Ideally it should show some prompt like "Marked hdz_009 as hot"

lets see that we can this merged as it currently stands ;) plenty of changes as is. further expansion is always an option.