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

Replace LIBAV with original FFMPEG #974

Closed
sgothel opened this Issue Nov 2, 2017 · 23 comments

Comments

@sgothel
Copy link
Contributor

sgothel commented Nov 2, 2017

Feature Request

While working on NVENC support #259
I have compared FFMPEG with LIBAV a little, backporting FFMPEG's NVENC implementation
to LIBAV.

Here I have discovered a few artifacts:

  1. FFMPEGs NVENC version is more up-to-date and closer to the Nvenc spec,
    LIBAVs version is sort of outdated.

  2. Most enhancements are being produced for FFMPEG first, then merely backported to LIBAV

  1. Above is also visible with the backporting, i.e. the format writer doesn't work properly using NVENC
  1. Reasons for Debian (and hence Ubuntu) to revert to the original FFMPEG

HandBrake version (e.g., 1.0.0)

Master branch

Example

Commit sgothel@0466852
as used for the NVENC feature in branch https://github.com/sgothel/HandBrake/tree/nvenc-encoder
(pull request #971)

I could easily replace LIBAV 12.2 with FFMPEG 3.4, however, one would need to
re-validated the many LIBAV patches for FFMPEG 3.4 - if they still apply.
A first attempt disclosed multiple rejections and this task surely needs some effort.

Work Assignment

I would volunteer performing this work, if the transition to FFMPEG is supported.

This was referenced Nov 2, 2017

@galad87

This comment has been minimized.

Copy link
Contributor

galad87 commented Nov 3, 2017

I have nothing against switching to FFmpeg. Other developers opinions may not be the same as mine.

@sr55 sr55 added the Enhancement label Nov 3, 2017

@sgothel

This comment has been minimized.

Copy link
Contributor Author

sgothel commented Nov 4, 2017

@sr55 sr55 added this to the 1.2.0 milestone Nov 6, 2017

@sr55

This comment has been minimized.

Copy link
Member

sr55 commented Nov 6, 2017

We'll want to push 1.1 out the door (hopefully sooner rather than later) then we can look at what's involved in switching over in 1.2. We don't really want to rock the boat right now quite close to finishing up work that will allow us to release.

1.2 can focus on decode/encode improvements as a key point.

While personally, I'd rather stay with libav, they are falling behind in certain areas that interest us so I'm not going to object to a switch over. If we had more time ourselves to maintain and backport what we need, it might be a different story but that's not going to happen realistically.

@jstebbins

This comment has been minimized.

Copy link
Contributor

jstebbins commented Nov 6, 2017

I currently know of only one incompatibility between libav and ffmpeg. libav uses AV_CODEC_ID_SRT for SRT subtitles. ffmpeg uses AV_CODEC_ID_SUBRIP, even though they also have a define for AV_CODEC_ID_SRT. Since they have the define, HandBrake builds without error against ffmpeg. But it fails to recognize SRT subtitles.

I wouldn't be surprised if there are more such problems waiting for us to discover.

Some of our current libav patches already made it into libav git. And many of those have gotten into ffmpeg due to the libav merge effort ffmpeg continues. But we'll have to inspect the patches carefully because ffmpeg often decides to do things a little differently.

@sgothel

This comment has been minimized.

Copy link
Contributor Author

sgothel commented Nov 6, 2017

@jstebbins yes ofc, that would be one important task for the ffmpeg move.
@sr55 how about creating a 1.2 branch and moving to ffmpeg on master .. or within its own branch?
Then you could merge my branch and we could work from there.

@sgothel

This comment has been minimized.

Copy link
Contributor Author

sgothel commented Nov 6, 2017

Maybe you like to have a clean FFMPEG branch of mine to merge w/ NVENC changes on top?

@jstebbins

This comment has been minimized.

Copy link
Contributor

jstebbins commented Nov 6, 2017

An ffmpeg switch should be done as it's own PR. Any PR that needs to be built on top of an ffmpeg switch can rebase or merge with the ffmpeg switch PR as necessary.

sgothel added a commit to sgothel/HandBrake that referenced this issue Nov 7, 2017

FFMPEG HandBrake#974: Use latest FFMPEG 3.4
Patch for HandBrake#974
moving to FFMPEG 3.4 from LIBAV 12.2.

All patches have been moved to subfolder 'old' since they do not apply cleanly anymore.
Work has to be performed to validate whether patches are still required.

The re-ordering of to be linked modules was required to solve
statically linked dependencies. See libhb/module.defs etc.
@sgothel

This comment has been minimized.

Copy link
Contributor Author

sgothel commented Nov 7, 2017

@jstebbins @sr55 I have re-packaged the work

  • ffmpeg pull req #981
  • nvenc pull req #983 (on top of ffmpeg pull req)

sgothel added a commit to sgothel/HandBrake that referenced this issue Dec 26, 2017

FFMPEG HandBrake#974: Use latest FFMPEG 3.4.1 (1/2)
Patch 1/2 for HandBrake#974
moving to FFMPEG 3.4.1 from LIBAV 12.2.

The re-ordering of to be linked modules was required to solve
statically linked dependencies. See libhb/module.defs etc.

sgothel added a commit to sgothel/HandBrake that referenced this issue Dec 26, 2017

FFMPEG HandBrake#974: Use latest FFMPEG 3.4.1 (2/2)
Patch 2/2 for HandBrake#974
moving to FFMPEG 3.4.1 from LIBAV 12.2.

All patches have been moved to subfolder 'old' since they do not apply cleanly anymore.
Work has to be performed to validate whether patches are still required.
@sgothel

This comment has been minimized.

Copy link
Contributor Author

sgothel commented Dec 26, 2017

@jstebbins I would commit on the swresample move 'as we go' within 2018, if that is fine with you.
Background: We have identified that avresample is deprecated in ffmpeg and therefor code shall use swresample 'soon'.
While it is debatable how high this priority is, it shall be done at some point in time.

@sgothel

This comment has been minimized.

Copy link
Contributor Author

sgothel commented Dec 26, 2017

Regarding the libav patches, see @cehoyos #981 (comment)

A11 needs another look.

A01-matroskaenc-pgs-duration.patch: Looks ok, sent to ffmpeg-devel
A04-dv-eof.patch: In FFmpeg since September 2015
A05-p10-output-support.patch: In FFmpeg since August 2016 (missing copyright btw)
A06-edit-list-offset.patch: I don't know of open edit-list issues in FFmpeg, if you find them, please tell me!
A09-mkv-block-ts-offset.patch: Merged into FFmpeg a month ago
A10-mp4-aac-roll.patch: Something similar looking is in FFmpeg
A11-elst-audio-pad.patch: Needs a bug report or a patch
A13-mkv-fix-seek-crash.patch: Fixed in FFmpeg November 2016
A14-aacenc-high-bitrate-crash.patch: Not relevant for FFmpeg (and fixed a long time ago)
A17-mkv-rawvideo.patch: Fixed in June 2011
A18-avi-broken-index.patch: Fixed in 2012
A19-h264-refs.patch: FFmpeg-patch from 2011
A20-hvc1-override.patch: In FFmpeg since June
P01-solaris.patch: Looks hardly relevant
P02-darwin-pic.patch: What's wrong with --enable-pic?

@sgothel

This comment has been minimized.

Copy link
Contributor Author

sgothel commented Dec 26, 2017

Highest libav/ffmpeg incompatibility impact might be the subtitle ID handling https://trac.ffmpeg.org/ticket/6304
See @cehoyos #981 (comment)

@jstebbins

This comment has been minimized.

Copy link
Contributor

jstebbins commented Dec 26, 2017

Although it would be nice to maintain compatibility with libav, I don't consider it practical. Trying to maintain compatibility would result in the same issues we were deliberately avoiding by not providing official support for ffmpeg. HandBrake does not have enough developers to support both.

So assuming compatibility is not required, you can just change AV_CODEC_ID_SRT to AV_CODEC_ID_SUBRIP in HandBrake and that should fix it.

@cehoyos

This comment has been minimized.

Copy link

cehoyos commented Dec 28, 2017

libavresample is not deprecated in FFmpeg yet and will iirc (sadly) not be removed before 2020, I sincerely hope it does no affect your patch.
The more important question is: Did you try to reproduce FFmpeg ticket 6304? I hope you agree that without a fix, your patch makes no sense.

Patch A01 was not committed so far: http://ffmpeg.org/pipermail/ffmpeg-devel/2017-November/219642.html

@sgothel

This comment has been minimized.

Copy link
Contributor Author

sgothel commented Dec 28, 2017

@cehoyos @jstebbins I have applied the tiny patch to HB FFMPEG as you can see above

@sgothel

This comment has been minimized.

Copy link
Contributor Author

sgothel commented Dec 28, 2017

patch A01 re-enabled in 'ffmpeg' branch

@jstebbins

This comment has been minimized.

Copy link
Contributor

jstebbins commented Dec 28, 2017

Regarding patches, Carl is correct in his review, although he skipped A12.

TLDR; neither A11 or A12 should be needed with ffmpeg.

Long answer. In libav A11 and A12 work together to make writing and reading back AAC sample accurate. Without these, the preroll in AAC would be read as extra samples that were not present in the original audio. These were required in libav because libav does not do complete edit list processing. ffmpeg's more complete edit list processing appears to handle all of this although I haven't tested.

@sgothel

This comment has been minimized.

Copy link
Contributor Author

sgothel commented Jan 12, 2018

@jstebbins when is HB 1.1 due?
Understanding your direction, let's see what time allows until.

@bradleysepos

This comment has been minimized.

Copy link
Member

bradleysepos commented Jan 12, 2018

HandBrake 1.1.0 is essentially done. We're trying to iron out a few graphical interface nits. See #833

@scaronni

This comment has been minimized.

Copy link

scaronni commented May 16, 2018

Still getting the Subtitle codec 94212 is not supported error message with some UTF-8 subtitles even with the patch at https://trac.ffmpeg.org/ticket/6304

@jstebbins

This comment has been minimized.

Copy link
Contributor

jstebbins commented May 16, 2018

@scaronni you've found a separate issue. That trac ticket is about demuxing and decoding SRT subtitles. The issue you've found is muxing SSA subtitles. The mkv muxer is complaining because HandBrake uses AV_CODEC_ID_SSA which libav expects. But ffmpeg expects AV_CODEC_ID_ASS instead. So we need another modification to HandBrake to change AV_CODEC_ID_SSA to AV_CODEC_ID_ASS in libhb/muxavformat.c

@scaronni

This comment has been minimized.

Copy link

scaronni commented May 17, 2018

ah, perfect, thank you. Will try that.

@scaronni

This comment has been minimized.

Copy link

scaronni commented May 23, 2018

This seems to be working fine, thanks!

bradleysepos added a commit to sgothel/HandBrake that referenced this issue May 29, 2018

FFMPEG HandBrake#974: Use latest FFMPEG 3.4.1 (1/2)
Patch 1/2 for HandBrake#974
moving to FFMPEG 3.4.1 from LIBAV 12.2.

The re-ordering of to be linked modules was required to solve
statically linked dependencies. See libhb/module.defs etc.

bradleysepos added a commit to sgothel/HandBrake that referenced this issue May 29, 2018

FFMPEG HandBrake#974: Use latest FFMPEG 3.4.1 (2/2)
Patch 2/2 for HandBrake#974
moving to FFMPEG 3.4.1 from LIBAV 12.2.

All patches have been moved to subfolder 'old' since they do not apply cleanly anymore.
Work has to be performed to validate whether patches are still required.

bradleysepos added a commit to sgothel/HandBrake that referenced this issue May 29, 2018

bradleysepos added a commit to sgothel/HandBrake that referenced this issue May 29, 2018

@bradleysepos

This comment has been minimized.

Copy link
Member

bradleysepos commented May 29, 2018

Merged FFmpeg 4.0 into master. See #1078 (comment) for some comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.