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

task/progress: Add Progress Tracking to Transcode and Prepare #38

Merged
merged 14 commits into from
Jun 17, 2022

Conversation

figintern
Copy link
Collaborator

@figintern figintern commented Jun 6, 2022

#26 #40

Refactor ReportProgress logic to specify progress sub-window between 0 and 100 percent. Percentage progress of getCount/size is applied only to the sub-window of overall progress.

Track progress for transcode and process tasks using size of segments pushed relative to overall size of the file.

transcode/process and import/process tasks are 50-50 split between the progress bar, this is easily adjustable.


Makes Prepare sub-task failures fatal


Also fixes bug in probe.go which causes error when video pixel format could not be probed from file and is empty

@figintern figintern requested a review from a team as a code owner June 6, 2022 10:21
@figintern figintern self-assigned this Jun 6, 2022
@figintern figintern linked an issue Jun 6, 2022 that may be closed by this pull request
task/progress.go Outdated Show resolved Hide resolved
task/progress.go Outdated Show resolved Hide resolved
Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM with a couple of nits. Still hunting for the bug you mentioned though 🧐

task/prepare.go Outdated Show resolved Hide resolved
task/progress.go Outdated Show resolved Hide resolved
task/progress.go Outdated Show resolved Hide resolved
task/progress.go Outdated Show resolved Hide resolved
task/transcode.go Outdated Show resolved Hide resolved
task/transcode.go Outdated Show resolved Hide resolved
task/transcode.go Outdated Show resolved Hide resolved
task/prepare.go Outdated Show resolved Hide resolved
task/import.go Outdated Show resolved Hide resolved
@figintern figintern force-pushed the tq/task-progress branch 3 times, most recently from 1d5e9f3 to 46f4379 Compare June 17, 2022 02:02
task/export.go Outdated Show resolved Hide resolved
Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

Some minor change which is required

task/import.go Outdated
glog.Errorf("error preparing imported file assetId=%s err=%q", tctx.OutputAsset.ID, err)
// TODO: make these fatal once we're confident about prepare reliability
return "", nil
glog.Fatalf("error preparing imported file assetId=%s err=%q", tctx.OutputAsset.ID, err)
Copy link
Member

Choose a reason for hiding this comment

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

These should not be fatal to the whole process. They should only be fatal for the task.

So instead of a fatal log here you should just return the error instead of ignoring it. Otherwise the whole process will crash and every other unrelated task will fail with it as well.

task/prepare.go Show resolved Hide resolved
task/progress.go Show resolved Hide resolved
task/progress.go Outdated Show resolved Hide resolved
task/transcode.go Outdated Show resolved Hide resolved
task/transcode.go Outdated Show resolved Hide resolved
Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM! Some optional nits in remaining comments

@figintern figintern merged commit 4d1ae17 into main Jun 17, 2022
@figintern figintern deleted the tq/task-progress branch June 17, 2022 07:29
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.

Update task progress on transcode and prepare
3 participants