jasonjmcghee / rem

An open source approach to locally record and enable searching everything you view on your Mac.
https://rem.ing
MIT License
2.18k stars 60 forks source link

Fix chunk indexing #90

Closed cparish312 closed 1 month ago

cparish312 commented 1 month ago

Previously there was a bug when the remembering function didn't close elegantly. This caused there to be frames in the frames table with a chunkId one higher than the top number in the chunks table (since the chunk wasn't added to the chunks table yet). This caused an issue running the timeline since there was no associated file. As well, the next frames would be initiated with the same chunkId causing there to be (chunkId, offset) duplicates in the frames table.

Another possibility was the ffmpeg video compressing process would fail, this would cause the chunk to exist in the chunks table but the associated path would not exist of be corrupted.

Fixes: 1) Moved let _ = DatabaseManager.shared.startNewVideoChunk(filePath: outputPath) to the end of the processChunk() and check that the ffmpegProcess exited successfully 2) Changed getCurrentChunkId() in DB.swift to return the chunkId of the last inserted frame (in case the chunk doesn't get saved to the database). Also, now insert chunk with specified chunkId instead of auto increment. 3) Added a chunksFramesView and associated interfaces for only grabbing frames with a chunk in the chunks table

Testing: Using the timeline functionality after purposefully stopping rem while in remember mode. Did not notice any performance decrease on Apple M3 Max (hopefully can get tested on a different model)

https://github.com/jasonjmcghee/rem/assets/54302513/2aa2fb0b-e075-405e-8bb1-b6ca71c2a2d3

jasonjmcghee commented 1 month ago

Thank you and excited to dive in further / test- and I have an m1 air so that'll be an extra test case!

Seems like the right changes to me- here are some other cases / details that seem like they should be unaffected but worth checking:

cparish312 commented 1 month ago

I was seeing some weird behaviors (mostly noticing through watching the live log) when exiting the timeline and going from the search to the timeline. The biggest issue was a second recording thread starting which eventually caused a seg fault and rem to crash. I don't believe the chunk indexing changes impacted this but thought I would include the changes since small anyways.

P.S. The double recording was due to the onClose call within the TimelineView. I wasn't entirely sure what the intended functionality of this is (mentions thumbnail clicking which I thought was only relevant for search), so I left it alone and solved the issue within the enableRecording func.

jasonjmcghee commented 1 month ago

@cparish312 nice change! Could you do the same fix for on opening search? Assuming it suffers from the same issue- double recording

jasonjmcghee commented 1 month ago

Thinking out loud- do we need to have a flag for whether we were recording before opening both timeline or search as if we open search and click a frame into timeline, will recording resume?

cparish312 commented 1 month ago

So before these changes clicking on a frame in search to enter timeline did start recording and then closing timeline caused a second thread to start recording. (At least on my end. Not sure how I could have introduced this but would be good to double check!)

Are you thinking a single variable for tracking if we were recording before either timleine or search? In that case, could probably have closeTimelineView() and closeSearchView() take an argument that ensure recording isn't started which could be passed when closing the search view within the timeline view.

jasonjmcghee commented 1 month ago

Does recording resume after recording -> search -> timeline -> closed with your changeset?

cparish312 commented 1 month ago

Yup! The newest change (adding userDisableRecording) ensures that this doesn't end in recording resuming recording -> timeline -> toggle recording off -> search -> closed

jasonjmcghee commented 1 month ago

ensures that this doesn't end in recording resuming

To clarify, recording should resume in that case

cparish312 commented 1 month ago

Hmm I'm a bit confused. By "toggle recording off" I mean that the user clicks "Stop Remembering" in the menu. I would assume that if that is selected remembering should not begin again after opening timeline or search and would require the user to click "Start Remembering" to resume recording.

jasonjmcghee commented 1 month ago

Totally true! But if you are recording when you open the timeline or search it "pauses" the recording and should automatically resume on close

cparish312 commented 1 month ago

Yup that is the functionality with the changes proposed!

jasonjmcghee commented 1 month ago

(so one more confirmation, sorry lol) if you are actively recording, open search, click a tile, which opens timeline, then close, which should resume recording.

cparish312 commented 1 month ago

No worries! Yup that is how it works! Implementation of that is this line at the top of showTimelineView() wasRecordingBeforeTimelineView = (isCapturing == .recording) || wasRecordingBeforeSearchView // handle going from search to TL

jasonjmcghee commented 1 month ago

I'll make sure to do more testing before building a new release, but merging!