Skip to content

Refactor FFmpegRecorder #103

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.

Sign up for GitHub

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

Draft
hiroTamada wants to merge 1 commit into
base: main
Choose a base branch
from
+34 −59
Draft

Refactor FFmpegRecorder #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 5 additions & 32 deletions server/cmd/api/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,29 +182,8 @@ func (s *ApiService) DownloadRecording(ctx context.Context, req oapi.DownloadRec

out, meta, err := rec.Recording(ctx)
if err != nil {
if errors.Is(err, recorder.ErrRecordingFinalizing) {
// Wait for finalization to complete instead of asking client to retry
log.Info("waiting for recording finalization", "recorder_id", recorderID)
ffmpegRec, ok := rec.(*recorder.FFmpegRecorder)
if !ok {
log.Error("failed to cast recorder to FFmpegRecorder", "recorder_id", recorderID)
return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "internal error"}}, nil
}
// WaitForFinalization blocks until finalization completes and returns the result
if finalizeErr := ffmpegRec.WaitForFinalization(ctx); finalizeErr != nil {
log.Error("finalization failed", "err", finalizeErr, "recorder_id", recorderID)
return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to finalize recording"}}, nil
}
// Finalization complete, retry getting the recording
out, meta, err = rec.Recording(ctx)
if err != nil {
log.Error("failed to get recording after finalization", "err", err, "recorder_id", recorderID)
return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to get recording"}}, nil
}
} else {
log.Error("failed to get recording", "err", err, "recorder_id", recorderID)
return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to get recording"}}, nil
}
log.Error("failed to get recording", "err", err, "recorder_id", recorderID)
return oapi.DownloadRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to get recording"}}, nil
}

// short-circuit if the recording is still in progress and the file is arbitrary small
Expand Down Expand Up @@ -246,15 +225,9 @@ func (s *ApiService) DeleteRecording(ctx context.Context, req oapi.DeleteRecordi
return oapi.DeleteRecording400JSONResponse{BadRequestErrorJSONResponse: oapi.BadRequestErrorJSONResponse{Message: "recording must be stopped first"}}, nil
}

if err := rec.Delete(ctx); err != nil {
if errors.Is(err, recorder.ErrRecordingFinalizing) {
log.Info("recording is being finalized, client should retry", "recorder_id", recorderID)
return oapi.DeleteRecording409JSONResponse{ConflictErrorJSONResponse: oapi.ConflictErrorJSONResponse{Message: "recording is being finalized, please retry in a few seconds"}}, nil
}
if !errors.Is(err, os.ErrNotExist) {
log.Error("failed to delete recording", "err", err, "recorder_id", recorderID)
return oapi.DeleteRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to delete recording"}}, nil
}
if err := rec.Delete(ctx); err != nil && !errors.Is(err, os.ErrNotExist) {
log.Error("failed to delete recording", "err", err, "recorder_id", recorderID)
return oapi.DeleteRecording500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "failed to delete recording"}}, nil
}

log.Info("recording deleted", "recorder_id", recorderID)
Expand Down
56 changes: 29 additions & 27 deletions server/lib/recorder/ffmpeg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,6 @@ const (
exitCodeProcessDoneMinValue = -1
)

// ErrRecordingFinalizing is returned when attempting to access a recording that is
// currently being finalized (remuxed to add duration metadata).
var ErrRecordingFinalizing = errors.New("recording is being finalized")

// FFmpegRecorder encapsulates an FFmpeg recording session with platform-specific screen capture.
// It manages the lifecycle of a single FFmpeg process and provides thread-safe operations.
type FFmpegRecorder struct {
Expand Down Expand Up @@ -237,14 +233,6 @@ func (fr *FFmpegRecorder) Stop(ctx context.Context) error {
return fr.finalizeRecording(ctx)
}

// WaitForFinalization blocks until finalization completes and returns the result.
// If finalization hasn't started, it will be triggered. If already complete, returns
// the cached result immediately. This is useful for callers like the download handler
// that need to wait for finalization before accessing the recording.
func (fr *FFmpegRecorder) WaitForFinalization(ctx context.Context) error {
return fr.finalizeRecording(ctx)
}

// ForceStop immediately terminates the recording process.
func (fr *FFmpegRecorder) ForceStop(ctx context.Context) error {
log := logger.FromContext(ctx)
Expand Down Expand Up @@ -378,22 +366,25 @@ func (fr *FFmpegRecorder) Metadata() *RecordingMetadata {
}

// Recording returns the recording file as an io.ReadCloser.
// Returns ErrRecordingFinalizing if the recording is currently being finalized.
// If the recording has exited but finalization is pending, this method blocks
// until finalization completes.
func (fr *FFmpegRecorder) Recording(ctx context.Context) (io.ReadCloser, *RecordingMetadata, error) {
fr.mu.Lock()
if fr.deleted {
fr.mu.Unlock()
return nil, nil, fmt.Errorf("recording deleted: %w", os.ErrNotExist)
}
// Block access while finalization is pending or in progress.
// This covers both the race window (after process exit, before finalization starts)
// and during active finalization.
if fr.exitCode >= exitCodeProcessDoneMinValue && !fr.finalizeComplete {
fr.mu.Unlock()
return nil, nil, ErrRecordingFinalizing
}
// If recording has exited but finalization is pending or in progress, wait for it.
// This hides finalization as an implementation detail from callers.
needsFinalization := fr.exitCode >= exitCodeProcessDoneMinValue && !fr.finalizeComplete
fr.mu.Unlock()

if needsFinalization {
if err := fr.finalizeRecording(ctx); err != nil {
return nil, nil, fmt.Errorf("failed to finalize recording: %w", err)
}
}

file, err := os.Open(fr.outputPath)
if err != nil {
return nil, nil, fmt.Errorf("failed to open recording file: %w", err)
Expand All @@ -416,18 +407,29 @@ func (fr *FFmpegRecorder) Recording(ctx context.Context) (io.ReadCloser, *Record
}

// Delete removes the recording file from disk.
// Returns ErrRecordingFinalizing if the recording is currently being finalized.
// If the recording has exited but finalization is pending, this method blocks
// until finalization completes before deleting.
func (fr *FFmpegRecorder) Delete(ctx context.Context) error {
fr.mu.Lock()
defer fr.mu.Unlock()
if fr.deleted {
fr.mu.Unlock()
return nil // already deleted
}
// Block deletion while finalization is pending or in progress.
// This covers both the race window (after process exit, before finalization starts)
// and during active finalization.
if fr.exitCode >= exitCodeProcessDoneMinValue && !fr.finalizeComplete {
return ErrRecordingFinalizing
// If recording has exited but finalization is pending or in progress, wait for it.
// We need to wait because deleting during finalization would corrupt the remux operation.
needsFinalization := fr.exitCode >= exitCodeProcessDoneMinValue && !fr.finalizeComplete
fr.mu.Unlock()

if needsFinalization {
// Wait for finalization to complete. We don't fail the delete if finalization
// fails - the caller wants the file gone regardless.
_ = fr.finalizeRecording(ctx)
}

fr.mu.Lock()
defer fr.mu.Unlock()
if fr.deleted {
return nil // deleted while we were waiting
}
if err := os.Remove(fr.outputPath); err != nil && !os.IsNotExist(err) {
return fmt.Errorf("failed to delete recording file: %w", err)
Expand Down
Loading