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

Error and retry chunk if frame count mismatches #612

Merged
merged 1 commit into from
Apr 17, 2022

Conversation

shssoichiro
Copy link
Collaborator

I can't think of a situation where someone would want this issue to be
ignored, so we should treat it as any other encoder error and perform 3
retries, then exit if it continues to fail. This probably indicates some
kind of issue with decoding or applying filters to that chunk.

@redzic
Copy link
Collaborator

redzic commented Apr 15, 2022

I think we also have to explicitly decrement the progress bar in the case of that type of failure.

@shssoichiro shssoichiro force-pushed the mismatch-error-retry branch from 5cf32a1 to ac5793d Compare April 15, 2022 01:50
);
} else {
error!(
"finished chunk: FRAME MISMATCH: chunk {}: {}/{} (actual/expected frames)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps this should be a warning message instead of an error since we are still restarting the chunk? I think this might be confusing to some users, what do you think though?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fair, it looks like in the other places we warn unless it's at max retries and bails.

@shssoichiro shssoichiro force-pushed the mismatch-error-retry branch from ac5793d to 475d13e Compare April 15, 2022 13:28
@shssoichiro
Copy link
Collaborator Author

I happened to have a file that got through the whole encode with one chunk with a significant frame mismatch. It muxed and happily deleted all of my temp files, and the resulting output had a significant desync after the broken chunk. So, just confirming that retrying after this and erroring after 3 failures is the correct approach.

@shssoichiro shssoichiro force-pushed the mismatch-error-retry branch 2 times, most recently from 0a2af17 to 8bc241a Compare April 15, 2022 17:48
@redzic
Copy link
Collaborator

redzic commented Apr 16, 2022

Looks much better without the code duplication, thanks! My only suggestion is to change line 250 of broker.rs to say "encoder failed" instead of "encoder crashed", to both match our warning message and to reflect our now more generic usage of EncoderCrash.

@shssoichiro shssoichiro force-pushed the mismatch-error-retry branch from 8bc241a to 3026017 Compare April 16, 2022 19:37
I can't think of a situation where someone would want this issue to be
ignored, so we should treat it as any other encoder error and perform 3
retries, then exit if it continues to fail. This probably indicates some
kind of issue with decoding or applying filters to that chunk.
@shssoichiro shssoichiro force-pushed the mismatch-error-retry branch from 3026017 to 4e12f0f Compare April 17, 2022 03:44
@mergify mergify bot merged commit 55df403 into master-of-zen:master Apr 17, 2022
@shssoichiro shssoichiro deleted the mismatch-error-retry branch April 17, 2022 03:55
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