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

Fix chunk indexing #90

Merged
merged 7 commits into from
Apr 29, 2024
Merged

Conversation

cparish312
Copy link
Contributor

@cparish312 cparish312 commented Apr 29, 2024

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)

Screen.Recording.2024-04-28.at.9.48.49.PM-1.mov

}
return 0
}

// Insert a new video chunk and return its ID
func startNewVideoChunk(filePath: String) -> Int64 {
let insert = videoChunks.insert(self.filePath <- filePath)
let insert = videoChunks.insert(self.id <- currentChunkId, self.filePath <- filePath)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could remove auto increment but I don't see the harm in keeping the requirements of increasing and unique

@jasonjmcghee
Copy link
Owner

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:

  • open timeline while recording (should pause it, and resume on closing timeline)
  • Same as above for search
  • sleep computer while remembering and wake it back up (should be seamless)

@jasonjmcghee jasonjmcghee mentioned this pull request Apr 29, 2024
@cparish312
Copy link
Contributor Author

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
Copy link
Owner

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

@jasonjmcghee
Copy link
Owner

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
Copy link
Contributor Author

cparish312 commented Apr 29, 2024

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
Copy link
Owner

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

@cparish312
Copy link
Contributor Author

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

@jasonjmcghee
Copy link
Owner

ensures that this doesn't end in recording resuming

To clarify, recording should resume in that case

@cparish312
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

Yup that is the functionality with the changes proposed!

@jasonjmcghee
Copy link
Owner

(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
Copy link
Contributor Author

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
Copy link
Owner

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

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.

2 participants