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.

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

Jump to bottom

Add patch fixing up segment muxer #10

Merged
merged 2 commits into from
Oct 2, 2019
Merged

Add patch fixing up segment muxer #10

merged 2 commits into from
Oct 2, 2019

Conversation

joshuaboniface
Copy link
Member

@joshuaboniface joshuaboniface commented Oct 2, 2019 •
edited
Loading

Closes #9
Fixes jellyfin/jellyfin#1694

Re-implementation of #9 as a Debian source patch (rather than a manual patch). Also includes the porting of this patch to the Docker ffmpeg build.

Manual patch required until the relevant patch is accepted upstream. Tracking at http://ffmpeg.org/pipermail/ffmpeg-devel/2019-October/250920.html

@nvllsvm nvllsvm self-requested a review October 2, 2019 18:14
nvllsvm
nvllsvm requested changes Oct 2, 2019
View reviewed changes
Copy link
Member

@nvllsvm nvllsvm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

nit - the commit messages aren't clear why the patch is needed. Also lacks a link to the upstream contribution.

Patch segment muxer in ffmpeg

This is necessary to fix xyz until it is fixed upstream.
Upstream contribution can be tracked at.

@joshuaboniface joshuaboniface requested a review from nvllsvm October 2, 2019 18:26
@nvllsvm
Copy link
Member

nvllsvm commented Oct 2, 2019

@joshuaboniface commit message, not pull request description. Pull request messages aren't included in the actual git repository AFAIK.

Use git commit --amend and/or git rebase - then force push to your branch.

@joshuaboniface
Copy link
Member Author

joshuaboniface commented Oct 2, 2019 •
edited
Loading

@nvllsvm Yes they are, the merge commit includes the full text of the PR description.

Or, not, I thought it did. That's stupid.

JustAMan
JustAMan approved these changes Oct 2, 2019
View reviewed changes
docker/Dockerfile
RUN \
DIR=/tmp/ffmpeg && mkdir -p ${DIR} && cd ${DIR} && \
for patch in *.patch; do \
filename="$( grep '+++' ${patch} | awk '{ print $2 }' | sed 's|jellyfin-ffmpeg/||' )"; \
patch --verbose ${filename} ${patch}; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, why not just patch -p1 < "$patch"?

Copy link
Member Author

Choose a reason for hiding this comment

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

For whatever reason, the patch command wasn't liking the Debian patch format, mostly because the actual path to the file was different (we're one level lower at this stage). This makes it super explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

But that is what -pN does - it drops first N dirs in a path before applying the patch...

Copy link
Contributor

Choose a reason for hiding this comment

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

Also your method would fail for multi-file patches...

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh really? OK, yea, that's nicer. I went through the man page quickly and didn't see the obvious solution. I guess that's an improvement to be made if we implement any more patches ;-) Hopefully we don't and this is redundant before then anyways.

@JustAMan JustAMan mentioned this pull request Oct 2, 2019
Closed
@joshuaboniface joshuaboniface merged commit da7926e into jellyfin Oct 2, 2019
@joshuaboniface joshuaboniface deleted the segment-muxer-fix branch October 2, 2019 22:10
@JustAMan
Copy link
Contributor

JustAMan commented Oct 3, 2019

For future reference, here's better upstream tracker URL: https://patchwork.ffmpeg.org/patch/15492/

@bibitocarlos bibitocarlos mentioned this pull request Mar 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustAMan JustAMan JustAMan approved these changes

@nvllsvm nvllsvm nvllsvm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Video playback will end prematurely
3 participants
@joshuaboniface @nvllsvm @JustAMan