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

Add compatibility with FFMPEG 7.0 #954

Merged
merged 4 commits into from
May 11, 2024

Conversation

eclipseo
Copy link
Contributor

@eclipseo eclipseo commented May 8, 2024

channel_layout has been replaced with ch_layout

@eclipseo eclipseo force-pushed the fix_for_ffmpeg7 branch 2 times, most recently from 44d5ee5 to 08ffdc4 Compare May 8, 2024 14:34
@eclipseo eclipseo force-pushed the fix_for_ffmpeg7 branch 2 times, most recently from ff78c63 to 6474572 Compare May 8, 2024 14:46
Copy link

codecov bot commented May 9, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 54.91%. Comparing base (a9e34a9) to head (5305e77).

Files Patch % Lines
src/FFmpegWriter.cpp 83.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #954      +/-   ##
===========================================
+ Coverage    54.26%   54.91%   +0.65%     
===========================================
  Files          182      182              
  Lines        16505    15948     -557     
===========================================
- Hits          8956     8758     -198     
+ Misses        7549     7190     -359     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/FFmpegReader.cpp Outdated Show resolved Hide resolved
src/FFmpegReader.cpp Outdated Show resolved Hide resolved
src/FFmpegWriter.cpp Outdated Show resolved Hide resolved
src/FFmpegWriter.cpp Outdated Show resolved Hide resolved
@jonoomph
Copy link
Member

jonoomph commented May 9, 2024

@eclipseo Thanks a bunch for taking a stab at this! I've left a few comments for you (sorry for the nitpicks). Mostly looks good, just a few questions and a few ideas to keep the changes tidy. 👍

@jonoomph
Copy link
Member

jonoomph commented May 9, 2024

@eclipseo eclipseo force-pushed the fix_for_ffmpeg7 branch 4 times, most recently from 104b9e8 to 85ab89f Compare May 9, 2024 09:55
channel_layout has been replaced with ch_layout

Fix OpenShot#953
@eclipseo
Copy link
Contributor Author

eclipseo commented May 9, 2024

So, we no longer need to set c->channels manually? I'm assuming this gets set automatically when channel_layout is assigned below? (in the newer code path)

Yes ch_layour is a structure that contains both the number of channels and the layout at the same time.

I updated various thing according to your notes, could check it?

@jonoomph
Copy link
Member

jonoomph commented May 9, 2024

@eclipseo Thanks! Your changes look great, much easier to follow. I'll do some additional testing on this today, just to verify no issues arise from our build servers, and then hopefully get it merged.

@jonoomph
Copy link
Member

jonoomph commented May 9, 2024

@eclipseo Build error:

/home/runner/work/libopenshot/libopenshot/src/FFmpegWriter.cpp: In member function ‘AVStream* openshot::FFmpegWriter::add_audio_stream()’:
/home/runner/work/libopenshot/libopenshot/src/FFmpegWriter.cpp:1125:10: error: conflicting declaration ‘uint64_t channel_layout’
 1125 | uint64_t channel_layout;
      |          ^~~~~~~~~~~~~~
/home/runner/work/libopenshot/libopenshot/src/FFmpegWriter.cpp:1084:24: note: previous declaration as ‘const uint64_t channel_layout’
 1084 |         const uint64_t channel_layout = info.channel_layout;
      |                        ^~~~~~~~~~~~~~
/home/runner/work/libopenshot/libopenshot/src/FFmpegWriter.cpp:1136:20: error: assignment of read-only variable ‘channel_layout’
 1136 |     channel_layout = c->channel_layout;
      |     ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
gmake[2]: *** [src/CMakeFiles/openshot.dir/build.make:398: src/CMakeFiles/openshot.dir/FFmpegWriter.cpp.o] Error 1
gmake[2]: Leaving directory '/home/runner/work/libopenshot/libopenshot/build'
gmake[1]: *** [CMakeFiles/Makefile2:1810: src/CMakeFiles/openshot.dir/all] Error 2
gmake[1]: Leaving directory '/home/runner/work/libopenshot/libopenshot/build'
gmake: *** [Makefile:166: all] Error 2

Moving variable init and fixing build error
2nd Attempt to fix this in the GitHub file editor, lol
attempt OpenShot#3 on GitHub editor, build error fix
@jonoomph jonoomph merged commit 7104dae into OpenShot:develop May 11, 2024
8 checks passed
@eclipseo
Copy link
Contributor Author

Ha my bad I had reused a variable name.

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.

2 participants