* [FFmpeg-devel] [PATCH] configure: disable vulkan if min version insufficient @ 2023-09-29 13:52 Tristan Matthews 2023-09-29 17:37 ` Timo Rothenpieler 2023-09-29 18:32 ` Hendrik Leppkes 0 siblings, 2 replies; 7+ messages in thread From: Tristan Matthews @ 2023-09-29 13:52 UTC (permalink / raw) To: ffmpeg-devel; +Cc: Tristan Matthews Fixes: https://trac.ffmpeg.org/ticket/10596 --- configure | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/configure b/configure index 20db1801ed..50ba6f772f 100755 --- a/configure +++ b/configure @@ -7154,7 +7154,8 @@ enabled crystalhd && check_lib crystalhd "stdint.h libcrystalhd/libcrystalhd_if. if enabled vulkan; then check_pkg_config_header_only vulkan "vulkan >= 1.3.255" "vulkan/vulkan.h" "defined VK_VERSION_1_3" || - check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" + check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" || + warn "Disabling vulkan" && disable vulkan fi if enabled x86; then -- 2.39.2 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH] configure: disable vulkan if min version insufficient 2023-09-29 13:52 [FFmpeg-devel] [PATCH] configure: disable vulkan if min version insufficient Tristan Matthews @ 2023-09-29 17:37 ` Timo Rothenpieler 2023-09-29 18:10 ` Tristan Matthews 2023-09-29 18:32 ` Hendrik Leppkes 1 sibling, 1 reply; 7+ messages in thread From: Timo Rothenpieler @ 2023-09-29 17:37 UTC (permalink / raw) To: ffmpeg-devel On 29.09.2023 15:52, Tristan Matthews wrote: > Fixes: https://trac.ffmpeg.org/ticket/10596 > --- > configure | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/configure b/configure > index 20db1801ed..50ba6f772f 100755 > --- a/configure > +++ b/configure > @@ -7154,7 +7154,8 @@ enabled crystalhd && check_lib crystalhd "stdint.h libcrystalhd/libcrystalhd_if. > > if enabled vulkan; then > check_pkg_config_header_only vulkan "vulkan >= 1.3.255" "vulkan/vulkan.h" "defined VK_VERSION_1_3" || > - check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" > + check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" || > + warn "Disabling vulkan" && disable vulkan > fi Doesn't that just always disable vulkan if any of the previous checks succeed? The logic looks weird to me. Also, shouldn't all of those check_* calls disable vulkan already? That's what the first argument is for. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH] configure: disable vulkan if min version insufficient 2023-09-29 17:37 ` Timo Rothenpieler @ 2023-09-29 18:10 ` Tristan Matthews 2023-09-29 19:26 ` Timo Rothenpieler 0 siblings, 1 reply; 7+ messages in thread From: Tristan Matthews @ 2023-09-29 18:10 UTC (permalink / raw) To: FFmpeg development discussions and patches On Fri, Sep 29, 2023 at 1:37 PM Timo Rothenpieler <timo@rothenpieler.org> wrote: > > On 29.09.2023 15:52, Tristan Matthews wrote: > > Fixes: https://trac.ffmpeg.org/ticket/10596 > > --- > > configure | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/configure b/configure > > index 20db1801ed..50ba6f772f 100755 > > --- a/configure > > +++ b/configure > > @@ -7154,7 +7154,8 @@ enabled crystalhd && check_lib crystalhd "stdint.h libcrystalhd/libcrystalhd_if. > > > > if enabled vulkan; then > > check_pkg_config_header_only vulkan "vulkan >= 1.3.255" "vulkan/vulkan.h" "defined VK_VERSION_1_3" || > > - check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" > > + check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" || > > + warn "Disabling vulkan" && disable vulkan > > fi > > Doesn't that just always disable vulkan if any of the previous checks > succeed? The logic looks weird to me. No, it will only disable vulkan if all the previous checks fail as this conditional shortcircuits (so to get to here it would have to be false || false || false || false || false || warn "Disable vulkan" && disable vulkan). If I hack the version number to match mine (changing it to 1.3.239), vulkan won't be disabled (that's why I added the warning message) and you'll hit the build breakage I and others are hitting currently. > > Also, shouldn't all of those check_* calls disable vulkan already? > That's what the first argument is for. They don't, so my first thought was that since this is the only place where check_pkg_config_header_only is called, it seemed likely that there was a bug in that function. However, I couldn't spot anything obviously broken in it and it is returning the correct value (hence this patch behaving as expected). Best, Tristan > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH] configure: disable vulkan if min version insufficient 2023-09-29 18:10 ` Tristan Matthews @ 2023-09-29 19:26 ` Timo Rothenpieler 2023-09-29 20:09 ` Tristan Matthews 0 siblings, 1 reply; 7+ messages in thread From: Timo Rothenpieler @ 2023-09-29 19:26 UTC (permalink / raw) To: ffmpeg-devel On 29.09.2023 20:10, Tristan Matthews wrote: > On Fri, Sep 29, 2023 at 1:37 PM Timo Rothenpieler <timo@rothenpieler.org> wrote: >> >> On 29.09.2023 15:52, Tristan Matthews wrote: >>> Fixes: https://trac.ffmpeg.org/ticket/10596 >>> --- >>> configure | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/configure b/configure >>> index 20db1801ed..50ba6f772f 100755 >>> --- a/configure >>> +++ b/configure >>> @@ -7154,7 +7154,8 @@ enabled crystalhd && check_lib crystalhd "stdint.h libcrystalhd/libcrystalhd_if. >>> >>> if enabled vulkan; then >>> check_pkg_config_header_only vulkan "vulkan >= 1.3.255" "vulkan/vulkan.h" "defined VK_VERSION_1_3" || >>> - check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" >>> + check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" || >>> + warn "Disabling vulkan" && disable vulkan >>> fi >> >> Doesn't that just always disable vulkan if any of the previous checks >> succeed? The logic looks weird to me. > > No, it will only disable vulkan if all the previous checks fail as > this conditional shortcircuits (so to get to here it would have to be > false || false || false || false || false || warn "Disable vulkan" && > disable vulkan). If I hack the version number to match mine (changing > it to 1.3.239), vulkan won't be disabled (that's why I added the > warning message) and you'll hit the build breakage I and others are > hitting currently. My shell disagrees: $ function warn() { echo $1; } $ true || false || warn "Disabling vulkan" && echo DISABLED DISABLED $ false || true || warn "Disabling vulkan" && echo DISABLED DISABLED $ false || false || warn "Disabling vulkan" && echo DISABLED Disabling vulkan DISABLED $ true || true || warn "Disabling vulkan" && echo DISABLED DISABLED No matter the results of the checks, this will always disable Vulkan, since the chain before it will always have a positive outcome, so && continues. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH] configure: disable vulkan if min version insufficient 2023-09-29 19:26 ` Timo Rothenpieler @ 2023-09-29 20:09 ` Tristan Matthews 0 siblings, 0 replies; 7+ messages in thread From: Tristan Matthews @ 2023-09-29 20:09 UTC (permalink / raw) To: FFmpeg development discussions and patches On Fri, Sep 29, 2023 at 3:26 PM Timo Rothenpieler <timo@rothenpieler.org> wrote: > > On 29.09.2023 20:10, Tristan Matthews wrote: > > On Fri, Sep 29, 2023 at 1:37 PM Timo Rothenpieler <timo@rothenpieler.org> wrote: > >> > >> On 29.09.2023 15:52, Tristan Matthews wrote: > >>> Fixes: https://trac.ffmpeg.org/ticket/10596 > >>> --- > >>> configure | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/configure b/configure > >>> index 20db1801ed..50ba6f772f 100755 > >>> --- a/configure > >>> +++ b/configure > >>> @@ -7154,7 +7154,8 @@ enabled crystalhd && check_lib crystalhd "stdint.h libcrystalhd/libcrystalhd_if. > >>> > >>> if enabled vulkan; then > >>> check_pkg_config_header_only vulkan "vulkan >= 1.3.255" "vulkan/vulkan.h" "defined VK_VERSION_1_3" || > >>> - check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" > >>> + check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" || > >>> + warn "Disabling vulkan" && disable vulkan > >>> fi > >> > >> Doesn't that just always disable vulkan if any of the previous checks > >> succeed? The logic looks weird to me. > > > > No, it will only disable vulkan if all the previous checks fail as > > this conditional shortcircuits (so to get to here it would have to be > > false || false || false || false || false || warn "Disable vulkan" && > > disable vulkan). If I hack the version number to match mine (changing > > it to 1.3.239), vulkan won't be disabled (that's why I added the > > warning message) and you'll hit the build breakage I and others are > > hitting currently. > > My shell disagrees: > > $ function warn() { echo $1; } > $ true || false || warn "Disabling vulkan" && echo DISABLED > DISABLED > $ false || true || warn "Disabling vulkan" && echo DISABLED > DISABLED > $ false || false || warn "Disabling vulkan" && echo DISABLED > Disabling vulkan > DISABLED > $ true || true || warn "Disabling vulkan" && echo DISABLED > DISABLED > > No matter the results of the checks, this will always disable Vulkan, > since the chain before it will always have a positive outcome, so && > continues. Oh I see, I should have parens around the last bit, e.g.: false || true || (warn "Disabling vulkan" && echo DISABLED) but as Hendrik mentioned, the warning can be dropped in which case this would become: check_pkg_config_header_only vulkan ... || check_cpp_condition || disable vulkan As mentioned earlier in the thread if someone has a better idea of how to avoid explicitly calling disable vulkan like this (i.e. why isn't this already working) that would of course be preferable. Best, -t _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH] configure: disable vulkan if min version insufficient 2023-09-29 13:52 [FFmpeg-devel] [PATCH] configure: disable vulkan if min version insufficient Tristan Matthews 2023-09-29 17:37 ` Timo Rothenpieler @ 2023-09-29 18:32 ` Hendrik Leppkes 2023-09-29 19:06 ` Tristan Matthews 1 sibling, 1 reply; 7+ messages in thread From: Hendrik Leppkes @ 2023-09-29 18:32 UTC (permalink / raw) To: FFmpeg development discussions and patches On Fri, Sep 29, 2023 at 3:55 PM Tristan Matthews <tmatth@videolan.org> wrote: > > Fixes: https://trac.ffmpeg.org/ticket/10596 > --- > configure | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/configure b/configure > index 20db1801ed..50ba6f772f 100755 > --- a/configure > +++ b/configure > @@ -7154,7 +7154,8 @@ enabled crystalhd && check_lib crystalhd "stdint.h libcrystalhd/libcrystalhd_if. > > if enabled vulkan; then > check_pkg_config_header_only vulkan "vulkan >= 1.3.255" "vulkan/vulkan.h" "defined VK_VERSION_1_3" || > - check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" > + check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" || > + warn "Disabling vulkan" && disable vulkan > fi > This change doesn't seem right. If a feature is explicitly requested, we generally fail the build and don't just disable the feature (afterall the user wanted it to be on). If the feature is not explicitly requested, then it should not print a message. - Hendrik _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [FFmpeg-devel] [PATCH] configure: disable vulkan if min version insufficient 2023-09-29 18:32 ` Hendrik Leppkes @ 2023-09-29 19:06 ` Tristan Matthews 0 siblings, 0 replies; 7+ messages in thread From: Tristan Matthews @ 2023-09-29 19:06 UTC (permalink / raw) To: FFmpeg development discussions and patches On Fri, Sep 29, 2023 at 2:32 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > On Fri, Sep 29, 2023 at 3:55 PM Tristan Matthews <tmatth@videolan.org> wrote: > > > > Fixes: https://trac.ffmpeg.org/ticket/10596 > > --- > > configure | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/configure b/configure > > index 20db1801ed..50ba6f772f 100755 > > --- a/configure > > +++ b/configure > > @@ -7154,7 +7154,8 @@ enabled crystalhd && check_lib crystalhd "stdint.h libcrystalhd/libcrystalhd_if. > > > > if enabled vulkan; then > > check_pkg_config_header_only vulkan "vulkan >= 1.3.255" "vulkan/vulkan.h" "defined VK_VERSION_1_3" || > > - check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" > > + check_cpp_condition vulkan "vulkan/vulkan.h" "defined(VK_VERSION_1_4) || (defined(VK_VERSION_1_3) && VK_HEADER_VERSION >= 255)" || > > + warn "Disabling vulkan" && disable vulkan > > fi > > > > This change doesn't seem right. If a feature is explicitly requested, > we generally fail the build and don't just disable the feature > (afterall the user wanted it to be on). That is the case here, with or without this patch, on my system --enable-vulkan will fail as expected on: > ERROR: vulkan requested but not found The bug I'm trying to address is the autodetect case. I'm happy to drop the warning, I just wanted it to be obvious what was happening (but one could infer it from the list of modules that will be built). > If the feature is not > explicitly requested, then it should not print a message. > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-09-29 20:10 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-09-29 13:52 [FFmpeg-devel] [PATCH] configure: disable vulkan if min version insufficient Tristan Matthews 2023-09-29 17:37 ` Timo Rothenpieler 2023-09-29 18:10 ` Tristan Matthews 2023-09-29 19:26 ` Timo Rothenpieler 2023-09-29 20:09 ` Tristan Matthews 2023-09-29 18:32 ` Hendrik Leppkes 2023-09-29 19:06 ` Tristan Matthews
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel This inbox may be cloned and mirrored by anyone: git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \ ffmpegdev@gitmailbox.com public-inbox-index ffmpegdev Example config snippet for mirrors. AGPL code for this site: git clone https://public-inbox.org/public-inbox.git