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

Handle zero-frame (audio-only) segments at the start of a session #204

Merged
merged 2 commits into from
Aug 24, 2020

Conversation

jailuthra
Copy link
Contributor

Why?
For details check the audio-only bug this PR fixes #203

How?
We bypass the usual transcoding process for the first n-segments of a stream that do not have any frames, but do have a valid video substream present in the container format.
The bypassing is done by forcing the copy transcoder 8bc28e3 for such segments.

Testing Done:

  • Manually reproduced with a failing segment from prod
  • Reproduced the issue with a unit test c372f56
  • Fixed & verified the fix for both of the above ^

ffmpeg/ffmpeg.go Outdated Show resolved Hide resolved
ffmpeg/ffmpeg.go Outdated Show resolved Hide resolved
ffmpeg/ffmpeg.go Outdated Show resolved Hide resolved
ffmpeg/api_test.go Show resolved Hide resolved
ffmpeg/lpms_ffmpeg.c Show resolved Hide resolved
ffmpeg/ffmpeg.go Outdated Show resolved Hide resolved
@iameli
Copy link
Contributor

iameli commented Aug 20, 2020

Right on — testing this now against the MistServer environment where we were encountering the problem.

@jailuthra
Copy link
Contributor Author

Force-pushed the suggestions in review comments.

@yondonfu
Copy link
Member

Force-pushed the suggestions in review comments.

A few team members have found fixup commits to be helpful for the review process for go-livepeer since reviewers can easily view the diff that addresses a set of requested changes (instead of viewing the entire file(s)) and the contributor can just auto-squash the fixup commits during a rebase before the merge. We also have a GitHub action in go-livepeer that blocks merges when fixup commits have not been squashed. Perhaps some of these things could be useful in this repo as well.

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

LGTM!

@jailuthra jailuthra merged commit 7160ad6 into master Aug 24, 2020
@jailuthra jailuthra deleted the jl/onlyaudio2 branch August 24, 2020 11:36
@jailuthra
Copy link
Contributor Author

jailuthra commented Aug 24, 2020

Perhaps some of these things could be useful in this repo as well.

Ah TIL, I had no idea about fixup/squashing in github, will use it from now thanks!

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.

Handle audio-only segments
3 participants