diff mbox series

[FFmpeg-devel] libavdevice: fix compilation for Mac OS X 10.7-10.12, iOS < 11

Message ID 21D6DE63-338C-4D76-B3AB-8D47D3B583E2@hotmail.com
State New
Headers show
Series [FFmpeg-devel] libavdevice: fix compilation for Mac OS X 10.7-10.12, iOS < 11 | expand

Checks

Context Check Description
yinshiyou/configure_loongarch64 warning Failed to apply patch
andriy/configure_x86 warning Failed to apply patch

Commit Message

Erik Bråthen Solem Aug. 20, 2024, 10:43 p.m. UTC
avfoundation.m uses constants prefixed with AVMediaType on Mac OS X > 10.6.
In 10.7 through 10.12 their type was NSString*, but starting with 10.13 a
new AVMediaType struct type was introduced. In avfoundation.m, the function
getDevicesWithMediaType takes this struct as parameter, which breaks support
for Mac OS X 10.7 through 10.12. By typedef-ing AVMediaType to NSString* for
these versions, the code compiles. Prior to 10.15 the value is passed to a
function that takes AVMediaType on 10.13+ and NSString* on <= 10.12. The
same API change was introduced in iOS starting with iOS 11.

Signed-off-by: Erik Bråthen Solem <erikbsolem@hotmail.com>
---
libavdevice/avfoundation.m | 4 ++++
1 file changed, 4 insertions(+)

Comments

Marvin Scholz Aug. 20, 2024, 11:17 p.m. UTC | #1
On 21 Aug 2024, at 0:43, Erik Bråthen Solem wrote:

> avfoundation.m uses constants prefixed with AVMediaType on Mac OS X > 10.6.
> In 10.7 through 10.12 their type was NSString*, but starting with 10.13 a
> new AVMediaType struct type was introduced. In avfoundation.m, the function
> getDevicesWithMediaType takes this struct as parameter, which breaks support
> for Mac OS X 10.7 through 10.12. By typedef-ing AVMediaType to NSString* for
> these versions, the code compiles. Prior to 10.15 the value is passed to a
> function that takes AVMediaType on 10.13+ and NSString* on <= 10.12. The
> same API change was introduced in iOS starting with iOS 11.
>

Hi, thanks for the patch. Conceptually looks fine to me.

See my remark below:

> Signed-off-by: Erik Bråthen Solem <erikbsolem@hotmail.com>
> ---
> libavdevice/avfoundation.m | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
> index c5a09c6563..779bc767d6 100644
> --- a/libavdevice/avfoundation.m
> +++ b/libavdevice/avfoundation.m
> @@ -763,6 +763,10 @@ static int get_audio_config(AVFormatContext *s)
>     return 0;
> }
>
> +#if ((TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000) || (TARGET_OS_OSX && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300))
> +typedef NSString* AVMediaType;
> +#endif

I do not think this is the proper guard here? You want to check *_MAX_ALLOWED if you want to check the SDK version,
which is what controls these change (as the SDK changed) not the minimum version you target when compiling.

> +
> static NSArray* getDevicesWithMediaType(AVMediaType mediaType) {
> #if ((TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000) || (TARGET_OS_OSX && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500))
>     NSMutableArray *deviceTypes = nil;
> -- 
> 2.46.0
>
> _______________________________________________
> 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".
gnattu Aug. 21, 2024, 1:09 a.m. UTC | #2
> On Aug 21, 2024, at 07:17, epirat07@gmail.com wrote:
> 
> 
> 
> On 21 Aug 2024, at 0:43, Erik Bråthen Solem wrote:
> 
>> avfoundation.m uses constants prefixed with AVMediaType on Mac OS X > 10.6.
>> In 10.7 through 10.12 their type was NSString*, but starting with 10.13 a
>> new AVMediaType struct type was introduced. In avfoundation.m, the function
>> getDevicesWithMediaType takes this struct as parameter, which breaks support
>> for Mac OS X 10.7 through 10.12. By typedef-ing AVMediaType to NSString* for
>> these versions, the code compiles. Prior to 10.15 the value is passed to a
>> function that takes AVMediaType on 10.13+ and NSString* on <= 10.12. The
>> same API change was introduced in iOS starting with iOS 11.
>> 
> 
> Hi, thanks for the patch. Conceptually looks fine to me.
> 
> See my remark below:
> 
>> Signed-off-by: Erik Bråthen Solem <erikbsolem@hotmail.com>
>> ---
>> libavdevice/avfoundation.m | 4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
>> index c5a09c6563..779bc767d6 100644
>> --- a/libavdevice/avfoundation.m
>> +++ b/libavdevice/avfoundation.m
>> @@ -763,6 +763,10 @@ static int get_audio_config(AVFormatContext *s)
>>    return 0;
>> }
>> 
>> +#if ((TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000) || (TARGET_OS_OSX && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300))
>> +typedef NSString* AVMediaType;
>> +#endif
> 
> I do not think this is the proper guard here? You want to check *_MAX_ALLOWED if you want to check the SDK version,
> which is what controls these change (as the SDK changed) not the minimum version you target when compiling.

This depends on use case though. For example, compiling on high version SDK, but targeting low version SDK and selectively
load high version symbols during the runtime should not use *_MAX_ALLOWED because that will disable higher version symbols
at build time which prevents the usage of the runtime version check.

> 
>> +
>> static NSArray* getDevicesWithMediaType(AVMediaType mediaType) {
>> #if ((TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000) || (TARGET_OS_OSX && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500))
>>    NSMutableArray *deviceTypes = nil;
>> -- 
>> 2.46.0
>> 
>> _______________________________________________
>> 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 <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
Marvin Scholz Aug. 21, 2024, 1:30 a.m. UTC | #3
On 21 Aug 2024, at 3:09, Gnattu OC via ffmpeg-devel wrote:

>> On Aug 21, 2024, at 07:17, epirat07@gmail.com wrote:
>>
>>
>>
>> On 21 Aug 2024, at 0:43, Erik Bråthen Solem wrote:
>>
>>> avfoundation.m uses constants prefixed with AVMediaType on Mac OS X > 10.6.
>>> In 10.7 through 10.12 their type was NSString*, but starting with 10.13 a
>>> new AVMediaType struct type was introduced. In avfoundation.m, the function
>>> getDevicesWithMediaType takes this struct as parameter, which breaks support
>>> for Mac OS X 10.7 through 10.12. By typedef-ing AVMediaType to NSString* for
>>> these versions, the code compiles. Prior to 10.15 the value is passed to a
>>> function that takes AVMediaType on 10.13+ and NSString* on <= 10.12. The
>>> same API change was introduced in iOS starting with iOS 11.
>>>
>>
>> Hi, thanks for the patch. Conceptually looks fine to me.
>>
>> See my remark below:
>>
>>> Signed-off-by: Erik Bråthen Solem <erikbsolem@hotmail.com>
>>> ---
>>> libavdevice/avfoundation.m | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
>>> index c5a09c6563..779bc767d6 100644
>>> --- a/libavdevice/avfoundation.m
>>> +++ b/libavdevice/avfoundation.m
>>> @@ -763,6 +763,10 @@ static int get_audio_config(AVFormatContext *s)
>>>    return 0;
>>> }
>>>
>>> +#if ((TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000) || (TARGET_OS_OSX && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300))
>>> +typedef NSString* AVMediaType;
>>> +#endif
>>
>> I do not think this is the proper guard here? You want to check *_MAX_ALLOWED if you want to check the SDK version,
>> which is what controls these change (as the SDK changed) not the minimum version you target when compiling.
>
> This depends on use case though. For example, compiling on high version SDK, but targeting low version SDK and selectively
> load high version symbols during the runtime should not use *_MAX_ALLOWED because that will disable higher version symbols
> at build time which prevents the usage of the runtime version check.

What I meant here is that *_MAX_ALLOWED must be checked as this is about the SDK version, while the *_MIN_REQUIRED ones are
about the targeted version. You do not runtime-load a typedef, that is purely a compile-time thing.

So you would incorrectly do the typedef even if your SDK is more recent as nothing prevents you targeting say 10.12 with a
10.14 SDK.

>
>>
>>> +
>>> static NSArray* getDevicesWithMediaType(AVMediaType mediaType) {
>>> #if ((TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000) || (TARGET_OS_OSX && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500))
>>>    NSMutableArray *deviceTypes = nil;
>>> -- 
>>> 2.46.0
>>>
>>> _______________________________________________
>>> 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 <mailto:ffmpeg-devel@ffmpeg.org>
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org <mailto: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".
Erik Bråthen Solem Aug. 21, 2024, 6:54 p.m. UTC | #4
I have very little experience with mailing lists, my apologies if this
reply does not end up where it is supposed to be.

The Apple developer docs say that AVMediaType is a struct, but it looks
seems that in reality it is typedef-ed to NSString*, just with an extra
specifier NS_EXTENSIBLE_STRING_ENUM. Thus the constants introduced in
10.7 are NSString* both before and after 10.13 and it would likely not
be a problem if this typedef were to be compiled in on 10.13+, but I
want to avoid it if possible.

I agree that *_MAX_ALLOWED is probably what I was looking for and
wrote a simple test program using the 10.12 and 10.13 SDKs to verify
that. Using AVMediaType and compiling with the 10.12 SDK fails.
Compiling it with the 10.13 succeeds and runs as expected on my 10.9
system. Compiling it with AVMediaType typedef-ed to NSString* and
SDK 10.12 also works as expected.

Thanks for putting me on the right track! I will try to submit a new
version of the patch as a reply to this thread. Please let me know
if I should submit it as a new thread instead.
Erik Bråthen Solem Aug. 21, 2024, 7:23 p.m. UTC | #5
avfoundation.m uses constants prefixed with AVMediaType on Mac OS X > 10.6.
In 10.7 through 10.12 their type was NSString*, but starting with 10.13 a
new type alias AVMediaType was introduced. In avfoundation.m, the function
getDevicesWithMediaType takes this type as parameter, which breaks support
for Mac OS X 10.7 through 10.12. By typedef-ing AVMediaType to NSString*
when targetting SDK 10.12 or older, the code compiles. Prior to 10.15 the
value is passed to a function that takes AVMediaType on 10.13+ and
NSString* on <= 10.12. The same API change was introduced in iOS starting
with iOS 11. The AVMediaType alias is itself typedef-ed to NSString* with
an extra specifier in the newer SDKs.

Signed-off-by: Erik Bråthen Solem <erikbsolem@hotmail.com>
---
libavdevice/avfoundation.m | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
index c5a09c6563..1367b06ae2 100644
--- a/libavdevice/avfoundation.m
+++ b/libavdevice/avfoundation.m
@@ -763,6 +763,10 @@ static int get_audio_config(AVFormatContext *s)
    return 0;
}

+#if ((TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MAX_ALLOWED < 110000) || (TARGET_OS_OSX && __MAC_OS_X_VERSION_MAX_ALLOWED < 101300))
+typedef NSString* AVMediaType;
+#endif
+
static NSArray* getDevicesWithMediaType(AVMediaType mediaType) {
#if ((TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000) || (TARGET_OS_OSX && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500))
    NSMutableArray *deviceTypes = nil;
diff mbox series

Patch

diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
index c5a09c6563..779bc767d6 100644
--- a/libavdevice/avfoundation.m
+++ b/libavdevice/avfoundation.m
@@ -763,6 +763,10 @@  static int get_audio_config(AVFormatContext *s)
    return 0;
}

+#if ((TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000) || (TARGET_OS_OSX && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300))
+typedef NSString* AVMediaType;
+#endif
+
static NSArray* getDevicesWithMediaType(AVMediaType mediaType) {
#if ((TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000) || (TARGET_OS_OSX && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500))
    NSMutableArray *deviceTypes = nil;