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

Limit HW accel support to FFmpeg 3.4+ #437

Merged
merged 3 commits into from Feb 14, 2020

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Feb 10, 2020

As noted in #436, our hw-accel code was never really compatible with FFmpeg 3.2.

This PR also adds an FFmpeg 3.2 build to our Travis matrix, ensuring that builds against that version are tested in the future.

Fixes #436
Fixes #433

Add a new #define HAVE_HW_ACCEL, which is only set on FFmpeg 3.4+,
and use that to restrict the use of hw-accel features, leaving
IS_FFMPEG_3_2 to determine only whether code is compatible with
FFmpeg 3.2+.
@codecov-io
Copy link

codecov-io commented Feb 10, 2020

Codecov Report

Merging #437 into develop will not change coverage.
The diff coverage is 25%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #437   +/-   ##
========================================
  Coverage    42.37%   42.37%           
========================================
  Files          129      129           
  Lines        13276    13276           
========================================
  Hits          5626     5626           
  Misses        7650     7650
Impacted Files Coverage Δ
include/FFmpegUtilities.h 100% <ø> (ø) ⬆️
include/FFmpegReader.h 33.33% <ø> (ø) ⬆️
src/FFmpegReader.cpp 67.8% <ø> (ø) ⬆️
src/FFmpegWriter.cpp 61.29% <25%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56af4eb...4106b99. Read the comment docs.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Feb 10, 2020

I'm going to hold off a bit on merging this, in case others (@jonoomph? @eisneinechse?) want to take a different approach, or even fix the FFmpeg 3.2 hw-accel support. But AFAICT that would take quite a bit of work (too much to be worth it), because I've still found no evidence that our existing hw-accel code was ever actually compatible with FFmpeg 3.2 — the use of hw_device_ctx in the codec context makes that impossible.

@eisneinechse
Copy link
Collaborator

From the history of libavcodec I can see:
2017-02-13 - c1a5fca06f - lavc 57.80.100 - avcodec.h
Add AVCodecContext.hw_device_ctx.
That was after ffmpeg 3.2 was released. Therefore, setting the minimum required libavcodec to 57.80.100 from 57.64.101 would be right. Maybe using 57. 89.100 (ffmpeg 3.3) would be better or even using 3.4+.
I don't have a version with ffmpeg 3.2 anymore and cannot test the hw acceleration using this version anymore. The programming started with ffmpeg 3.2.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Feb 11, 2020

@eisneinechse I went with libavcodec 57.107.100, the version released as FFmpeg 3.4, since there's also the question of when AV_HWDEVICE_FORMAT_NONE was added to the AVHWDeviceType enum (also some time between 3.2 and 3.4), and any other differences there might be between the two versions.

Thanks to the CI system, we've been testing FFmpeg 3.4 all along (it's the version included in Ubuntu Bionic), but since there was no LTS Ubuntu release that included FFmpeg 3.2, it was never part of the testing infrastructure. (In fact, it was never even on our testing radar until yesterday.) So, I have a feeling code slipped in early on that wasn't compatible with FFmpeg 3.2 (but was compatible with 3.4), and since all the CI tests were still passing it understandably went unnoticed.

I figure, we might be able to support a slightly earlier version, but since we can't know for sure, we'd have to go through the hassle of testing to figure out exactly what that would be. And... for what? To support hardware-accel de/encoding on a few additional, very-out-of-date systems? Hardly seems worth the effort.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Feb 14, 2020

I'm going to merge this now, because @cwilling reported that it solves the build issues with FFmpeg 3.2 (as our own newly-upgraded CI matrix also demonstrates, with this merge), and hwaccel support for 3.2/3.3 can always be revisited/restored in a future PR.

@ferdnyc ferdnyc merged commit fe8ea21 into OpenShot:develop Feb 14, 2020
@ferdnyc ferdnyc deleted the hw-accel-ffmpeg34 branch February 14, 2020 17:11
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.

Our hardware-decoding code never supported FFmpeg 3.2 AV_HWDEVICE_TYPE_NONE was not declared
3 participants