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

Port to FFmpeg 5.x #204

Merged
merged 6 commits into from
Jan 7, 2023
Merged

Port to FFmpeg 5.x #204

merged 6 commits into from
Jan 7, 2023

Conversation

basilgello
Copy link
Contributor

Fixes #188

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>
Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>
Comment on lines -732 to -739
if (iformat && (strcmp(iformat->name, "mov,mp4,m4a,3gp,3g2,mj2") == 0))
{
CURL url(m_streamUrl);
//if (URIUtils::IsRemote(strFile))
if (!url.GetProtocol().empty() && !url.IsProtocol("file"))
m_pFormatContext->iformat->flags |= AVFMT_NOGENSEARCH;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AlwinEsch do you recall the reason for this?

@herrnst
Copy link
Member

herrnst commented Nov 14, 2022

Hi, not sure if its okay to already ask questions as even the FFMPEG 5 pull isn't merged into the Kodi repository yet, I'll try anyways:

I picked up current Kodi master (which will become Nexus), applied xbmc/xbmc#21248 and built/installed everything (worked without issues so far). This also installed the ffmpeg-related .a archives in /usr/lib/x864_64-linux-gnu/kodi/ffmpeg/lib/lib*.a and the headers below /usr/include/kodi/lib* (ie. libavcodec et al).

Then I picked up this PR branch (inputstream.ffmpegdirect+ffmpeg5) and invoked CMake with
-DCMAKE_PREFIX_PATH=/usr/lib/x864_64-linux-gnu/kodi/ffmpeg and CMake picks up FFmpeg from Kodi.

Compilation works, but at the end linking fails with

/usr/bin/ld: /usr/lib/x86_64-linux-gnu/kodi/ffmpeg/lib/libavcodec.a(h264_intrapred_10bit.o): warning: relocation against `ff_pw_512' in read-only section `.text'
/usr/bin/ld: /usr/lib/x86_64-linux-gnu/kodi/ffmpeg/lib/libavcodec.a(vc1dsp_mmx.o): relocation R_X86_64_PC32 against symbol `ff_pw_9' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: bad value
collect2: error: ld returned 1 exit status

This is on a minimal Ubuntu 22.04 install (used solely for packaging purposes) with all current updates installed. What am I doing wrong?

@basilgello
Copy link
Contributor Author

@herrnst How do you build? You build dsc from ppa?

@herrnst
Copy link
Member

herrnst commented Nov 14, 2022

@herrnst How do you build? You build dsc from ppa?

Not sure if the answer is "yes", basically I pick up packaging ("debian/") from xbmc-nightly at launchpad.net/~team-xbmc (https://launchpad.net/~team-xbmc/+archive/ubuntu/xbmc-nightly/+sourcepub/14089652/+listing-archive-extra) and use that to build from the GIT checkout (here with FFMPEG 5 patches applied) using dpkg-buildpackage with all deps installed. Same for inputstream.ffmpegdirect, so thats where kodi-ffmpeg-dev (ie. /usr/lib//libavcodec.a et al) come from.

So, short answer: debian-dir from PPA with GIT tree and debhelper/dh-make and friends, both for Kodi and inputstream.ffmpegdirect.

Fixes the transport stream playback failures described in
https://bugs.debian.org/1016925

@Rogo95 made an excellent technical analysis of the root cause
and reported that to the bug thread.

Later on, James Almer (@jamrial) suggested the solution to use
extract_extradata bitstream filter to replace the removed split()
function.

Finally, I adapted the following code snippet:
https://gist.github.com/moonpfe/f6795d51294d91ee0f82f62ff6985db0
to Kodi and tested it by playing the affected files in TS format.

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>
Team Kodi does officially support one major FFmpeg version.
This commit is intentionally separated to let *nix distro
maintainers revert it if building Kodi 20+ is needed with
FFmpeg 4.

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>
Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>
Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>
@basilgello basilgello closed this Dec 27, 2022
@basilgello basilgello reopened this Dec 27, 2022
@basilgello basilgello closed this Jan 6, 2023
@basilgello basilgello reopened this Jan 6, 2023
@basilgello
Copy link
Contributor Author

@phunkyfish The ffmpeg5 PR is merged into master. Can you please create Omega branch so I can rebase the PR into it? As I told previously, we need to use the same ffmpeg version across kodi and addons.

@phunkyfish
Copy link
Collaborator

Creating a new branch won’t work until all binary add-ons have branches switched from Nexus to Omega (but if you ask me master would be better).

I’m not sure we can branch for a single add-on and leave the others as is.

@basilgello
Copy link
Contributor Author

basilgello commented Jan 6, 2023

I am afraid that the ffmpeg5 upgrade on kodi core will break ffmpegdirect earlier built with bundled ffmpeg4 for most users. Windows users might not notice that as well as Debian's, but other Linux / OSX / Android / FreeBSD certainly will :(

As a workaround until the Omega repo goes prime time users can install the artifacts from this PR. In this case this PR should be held on until we have Omega branch.

@phunkyfish
Copy link
Collaborator

When ffmpeg 4 was released it was out for many months on ffmpegdirect before being in kodi. If it worked then, it could also work now with ffmpeg 5. But the only way to see if it does is if we try it.

@lrusak
Copy link
Contributor

lrusak commented Jan 7, 2023

Omega is currently pre-alpha so I wouldn't worry about breakage too much.

@phunkyfish
Copy link
Collaborator

phunkyfish commented Jan 7, 2023

The issue is we could break Nexus as the same version of the add-on would be used for both.

But let’s test both first and see what happens.

@basilgello
Copy link
Contributor Author

basilgello commented Jan 7, 2023

OK, did some tests with team's PPA for jammy:

  1. kodi 6:20.0+git20230105.0300-10f29c9fd4-0~jammy (ffmpeg4) + ffmpegdirect 20.5.0 with this PR (ffmpeg5) = works!
  2. kodi 6:20.0+git20230107.0300-8fdbdb1890-0~jammy (ffmpeg5) + ffmpegdirect 20.5.0 from PPA (ffmpeg4) = no picture and symptoms are just like the extradata

So we can merge this PR - and the faster we merge it, the less chance users will get combo number 2 :)

@phunkyfish phunkyfish merged commit 6652d3b into xbmc:Nexus Jan 7, 2023
@basilgello
Copy link
Contributor Author

And if we still support Matrix branch, the use-after-free commit likely applies to matrix as well…

@phunkyfish
Copy link
Collaborator

And if we still support Matrix branch, the use-after-free commit likely applies to matrix as well…

Please PR that change also and I’ll merge it.

@lrusak
Copy link
Contributor

lrusak commented Jan 7, 2023

The issue is we could break Nexus as the same version of the add-on would be used for both.

But let’s test both first and see what happens.

Ah, why bump to 5.0 for the Nexus version when Kodi Nexus is still 4.x? Is it not valid to keep them in sync?

@basilgello
Copy link
Contributor Author

basilgello commented Jan 7, 2023

That was my concern, too :) But things seem to work if core ffmpeg is older than addon's - if the linkage of the addon is static and there are no other binary addons trying to use dynamic ffmpeg library!

@lrusak
Copy link
Contributor

lrusak commented Jan 7, 2023

That was my concern, too :) But things seem to work if core ffmpeg is older than addon's - if the linkage of the addon is static and there are no other binary addons trying to use dynamic ffmpeg library!

Yea that's annoying. Now LibreELEC (and other distros) need to manage two ffmpeg versions and build them statically.

@basilgello
Copy link
Contributor Author

The alternative is to do quick omega branching and revert this commit :) I can help you with it tonight. And please upload,tzdata to mirrors, too!

@phunkyfish
Copy link
Collaborator

phunkyfish commented Jan 7, 2023

That was my concern, too :) But things seem to work if core ffmpeg is older than addon's - if the linkage of the addon is static and there are no other binary addons trying to use dynamic ffmpeg library!

Yea that's annoying. Now LibreELEC (and other distros) need to manage two ffmpeg versions and build them statically.

Why? Ffmpegdirect should be able to change the underlying ffmpeg version without impacting kodi.

@lrusak
Copy link
Contributor

lrusak commented Jan 7, 2023

That was my concern, too :) But things seem to work if core ffmpeg is older than addon's - if the linkage of the addon is static and there are no other binary addons trying to use dynamic ffmpeg library!

Yea that's annoying. Now LibreELEC (and other distros) need to manage two ffmpeg versions and build them statically.

Why? Ffmpegdirect should be able to change the underlying ffmpeg version without impacting kodi.

Until we allow using whatever ffmpeg version available in Kodi core distros will have to manage two ffmpeg versions.

I agree that the add-on should not need the exact version of ffmpeg that core uses but we aren't quite there yet.

@HiassofT
Copy link

HiassofT commented Jan 8, 2023

Just a quick heads up: if you start to require a different ffmpeg version than the fork kodi uses (eg 4.x for Nexus) then we (LE) might just choose to not build the ffmpegdirect addon any longer as it'll become too much hassle.

Ideally both kodi and ffmpegdirect should work with non-forked upstreeam ffmpeg versions (ideally both 4.x and 5.x - your choice) as provided by distros - that should be the mid-term goal both for kodi omega and LE12

@basilgello
Copy link
Contributor Author

basilgello commented Jan 8, 2023

@HiassofT I did everything to reduce hassle here :)

First of all, LE can build Nexus with ffmpeg5 (using the commit from Debian)

Another option is to add the patch (reverting 60ffdc3) here to build ffmpegdirect with ffmpeg4 (whipe preserving the ability to build with ffmpeg5 at the same time!).

Finally, when Omega gets branched in addons, we can revert this PR as a whole for Nexus. That sounds a proper long-term solution to me.

@lrusak
Copy link
Contributor

lrusak commented Jan 8, 2023

Finally, when Omega gets branched in addons, we can revert this PR as a whole for Nexus. That sounds a proper long-term solution to me.

I think this is the best option and should happen sooner than later.

Omega can be branched at any time. Nothing is holding that back really.

@razzeee
Copy link
Member

razzeee commented Jan 8, 2023

Why are we merging this to a nexus branch at all then? This is causing failing builds for no reason?

@phunkyfish
Copy link
Collaborator

Will revert later today.

@HiassofT
Copy link

HiassofT commented Jan 8, 2023

@basilgello We are not going to patch kodi Nexus to work with ffmpeg5, this will create a huge maintenance burden.

I, too, find it unfortunate that team kodi decided to ship Nexus with ffmpeg 4 but it is what it is and we have to live with that.

Kodi doesn't sandbox binary addons (eg into spearate processes) yet so you can't easily mix libraries - kodi's address space will already have ffmpeg 4 symbols present and trying to mix that with ffmpeg 5 is crying for problems (and likely very sublte issues that are going to be hard to track down).

I fully understand that you'll want to ship kodi Nexus with ffmpeg 5 on Debian but this means you're going to have to locally patch kodi and addons to do that.

@phunkyfish
Copy link
Collaborator

Reverted. Once binary add-ons all get branched for Omega we can apply this PR again.

@basilgello
Copy link
Contributor Author

I have no hard feelings about this but I recall there was a script posted by @fuzzard in slack to automate branching. What was the issue there?

@phunkyfish
Copy link
Collaborator

I did not see the script. But it will need someone with with the correct permissions to run it for all binary addons.

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.

Please add support for ffmpeg 5
6 participants