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

Rebase Livepeer changes to ffmpeg v7.0.1 #32

Merged
merged 12 commits into from
Jul 10, 2024

Conversation

j0sh
Copy link
Collaborator

@j0sh j0sh commented Jun 28, 2024

DO NOT SQUASH MERGE

Please preserve this list of commits for the next time we have to rebase to a newer ffmpeg. Do a rebase and merge, or merge commit instead

Description

Cherry picked the commits from the livepeer branch to ffmpeg v7.0.1

  • Omit all DNN-related changes since that is no longer used
  • Omit merge commits; cherry-picked the commits directly
  • Update to latest ffmpeg API, mostly around the signature_cuda filter
  • Fix merge conflicts related to us changing code that is still being actively maintained in ffmpeg (signature, nvenc)
  • Squash related commits together to make a tidier list of commits
  • Reorder some commits to make it easier to review later: nvenc changes are grouped and come earlier in the tree, followed by signature changes
  • Fix a new bug in ffmpeg with nvenc and intra-only - 80404f8

Post-Merge

  • The main branch of this repo should be set to n7.0.1-livepeer
  • Update install_ffmpeg.sh in go-livepeer, or even better swap with LPMS version of install_ffmpeg to avoid the current circular dependency

Merge Conflicts

Most merge conflicts were from us applying a patch to code that had also been changed upstream. This was usually relatively straightforward to resolve, but in the signature filter, we are moving big chunks of code around while that same code was being changed upstream. This meant carefully reviewing all upstream changes and ensuring that those were also reflected in our own (relocated) chunks of code. We need a better way to maintain these changes going forward (including maybe upstreaming the signature_cuda filter), because doing this for each ffmpeg update would be slow, tedious and very error-prone.

This was involved enough that I ended up writing a little script to help verify the changes by comparing a diff of the diffs of both sets of commits against each respective base branch. The tool is a little rough but it did catch a few issues so I am more comfortable with these changes.

Github display of commits

The way Github displays commits is completely wrong; they are listed in commit-date order [1] but this does not reflect the actual ordering of commits in-tree, nor the authorship date. Pull the branch via cli and git log to see the actual commit ordering and authorship dates.

image
git log --pretty=format:'%C(yellow)%h %Cred%ad %Cblue%an%Cgreen%d %Creset%s' --date=short

[1] cherry picking overwrites the commit date by default, but I can fix this up to preserve the original commit timestamp if preferred. Author timestamps are already correct, but github does not show this. git log does the correct thing and shows author timestamps.

jailuthra and others added 11 commits June 20, 2024 07:26
Recent encode API restructure (827d6fe) removed some state - which broke
the API for flushing without closing the encoder.

This functionality was originally added in 3ea7057 and is useful for
segmented video, where we don't want to do expensive re-init of HW sessions
for every segment.
In intra-only mode, frameIntervalP is 0, which means the frame
data array is smaller than the number of surfaces. This causes a
crash when closing the encoder.

Fix this by making sure the frame data array is at least as big as
the number of surfaces.
@j0sh j0sh mentioned this pull request Jul 1, 2024
6 tasks
@emranemran
Copy link

@j0sh where is the signature_cuda filter used?

@j0sh
Copy link
Collaborator Author

j0sh commented Jul 9, 2024

@j0sh where is the signature_cuda filter used?

No idea, all I know is the signature filter is still used (not sure by whom) ... unsure if that also means signature_cuda

If we can remove signature_cuda then that would simplify a lot of the conflicts here

@emranemran
Copy link

@thomshutt do you happen to know re: signature_cuda?

@thomshutt
Copy link

From the changelog:

This release includes a new orchestrator/transcoder MPEG-7 video signature capability which is required for broadcasters to run transcoding verification. The MPEG-7 video signatures are used as perceptual hashes in a "fast verification" algorithm that is described in the Fast and Full Transcoding Verification design. If a CPU is used for transcoding, the video signature computation will occur on the CPU. If a GPU is used for transcoding (i.e. the -nvidia flag is used), the video signature computation will occur on the GPU. Orchestrators and transcoders should upgrade to this release as soon as possible to ensure that they will be able to continue serving broadcasters that run fast verification (the complete broadcaster implementation will be shipped at a later date)

So it looks like it's part of "fast verification", which never made it to Production. The filter is still referenced in LPMS, but I'm happy for you to drop it from there.

@j0sh
Copy link
Collaborator Author

j0sh commented Jul 10, 2024

Added a new fix in 49b9f4b that should make segmenting more resilient; hopefully it will be incorporated upstream soon.

re: signature_cuda, sounds good we can remove it in the next round of updates - probably not worth trying to untangle that right now

Encoders may emit a buffering period SEI without a corresponding
SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc.

During Annex B conversion, this may result in the SPS/PPS being
inserted *after* the buffering period SEI but before the IDR NAL.

Since the buffering period SEI references the SPS, the SPS/PPS
needs to come first.
@j0sh j0sh force-pushed the n7.0.1-livepeer branch from 010ea1b to 49b9f4b Compare July 10, 2024 17:45
@j0sh j0sh merged commit bf7d883 into livepeer:n7.0.1-livepeer Jul 10, 2024
@j0sh j0sh deleted the n7.0.1-livepeer branch July 10, 2024 19:35
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.

8 participants