Skip to content

Update Dockerfile flags #625

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update Dockerfile flags #625

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9599f2
Update Dockerfile flags
gtsteffaniak Jun 1, 2025
2654c9b
Update Dockerfile
gtsteffaniak Jun 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1135,12 +1135,12 @@ RUN \
# workaround for https://gitlab.com/AOMediaCodec/SVT-AV1/-/merge_requests/2387
sed -i 's/svt_av1_enc_init_handle(&svt_enc->svt_handle, svt_enc, &svt_enc->enc_params)/svt_av1_enc_init_handle(\&svt_enc->svt_handle, \&svt_enc->enc_params)/g' libavcodec/libsvtav1.c && \
FDKAAC_FLAGS=$(if [[ -n "$ENABLE_FDKAAC" ]] ;then echo " --enable-libfdk-aac --enable-nonfree " ;else echo ""; fi) && \
sed -i 's/add_ldexeflags -fPIE -pie/add_ldexeflags -fPIE -static-pie/' configure && \
./configure \
--pkg-config-flags="--static" \
--extra-cflags="-fopenmp" \
--extra-cflags="$CFLAGS" \
--extra-cxxflags="$CXXFLAGS" \
--extra-ldexeflags="-fPIE -static-pie" \
--extra-ldflags="-fopenmp -Wl,--allow-multiple-definition -Wl,-z,stack-size=2097152" \
--toolchain=hardened \
Copy link
Owner

@wader wader Jun 1, 2025

Choose a reason for hiding this comment

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

Wonder if we should keep this if ffmpeg starts to use addition flags?

Copy link
Author

@gtsteffaniak gtsteffaniak Jun 1, 2025

Choose a reason for hiding this comment

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

well, looking at the configure for FFmpeg it seems like it was not valuable when we added our needed cflags manually.

Copy link
Author

@gtsteffaniak gtsteffaniak Jun 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

thats the only thing I removed it based on. previously, it would only work with hardened toolchain flag, but since it now works with the right cflags without hardened toolchain, I didn't want it messing up the cflags that worked. I can't remember if I tested it with hardened toolchain or not. I just know it worked without it.

Copy link
Owner

@wader wader Jun 1, 2025

Choose a reason for hiding this comment

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

huh, failed with hardened flag when building for armv7 or what was the build env? remember what/how it failed?

Copy link
Author

@gtsteffaniak gtsteffaniak Jun 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah this doesn't enable armv7, I wasn't able to get that working. I basically gave up on that, but this change came from that debugging. So I thought I would add this anyways

Copy link
Owner

@wader wader Jun 2, 2025

Choose a reason for hiding this comment

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

Sorry i'm still a bit confused what kind of build your trying to enable? i think it would be good to keep the hardened flag to ffmpeg configure if they ever add more flags or if they start using it to make ffmpeg's own source more fortified etc

Copy link
Author

@gtsteffaniak gtsteffaniak Jun 2, 2025

Choose a reason for hiding this comment

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

Nothing changed in behavior other than simplifying the build steps without the hack.

Copy link
Owner

@wader wader Jun 2, 2025

Choose a reason for hiding this comment

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

Still a bit confused :) so is it that --toolchain=hardened adds -pie and that does not play nice with --extra-ldexeflags="-fPIE -static-pie"?

Copy link
Author

@gtsteffaniak gtsteffaniak Jun 2, 2025

Choose a reason for hiding this comment

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

yeah, I assumed that's the reason you did the sed/hardened. Because it does -pie and that conflicts with -static-pie.

--toolchain=hardened also does:

add_cppflags -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
        add_cflags   -fno-strict-overflow -fstack-protector-all

which we could add explcitly if they are needed. But I'm not sure what those values are or how they are used.

Copy link
Owner

@wader wader Jun 2, 2025

Choose a reason for hiding this comment

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

Aha no i for some reason didn't see that --extra-ldexeflags existed so i used sed instead.

FORTIFY_SOURCE is a libc thing (possible other libs also?), to add additional checks etc. Now i actually checked and it seems musl is not using it but might do in the future.

-fno-strict-overflow -fstack-protector-all tells to compiler to generate code with various safe guards.

And i guess that the ffmpeg devs might add more or these in the future so it would be nice to automatically get them. Other option could be to from time to time sync the Dockerfile flags with what ffmpeg uses.

--disable-debug \
--disable-shared \
--disable-ffplay \
Expand Down