[FFmpeg-devel] lavc/videotoolboxenc: Fix compilation for IOS < 11.0 and OSX, < 10.13

Submitted by Thilo Borgmann on May 14, 2019, 1:15 p.m.

Details

Message ID 0499ea09-caa1-994e-1ce6-a15010bd81ad@mail.de
State New
Headers show

Commit Message

Thilo Borgmann May 14, 2019, 1:15 p.m.
$Subject

Tested compilation only, sanity test actually using it appreciated.

Thanks,
Thilo
From 104b26ca0eab116dcd49b5f2090067b76b5bfc70 Mon Sep 17 00:00:00 2001
From: Thilo Borgmann <thilo.borgmann@mail.de>
Date: Tue, 14 May 2019 15:11:38 +0200
Subject: [PATCH] lavc/videotoolboxenc: Fix compilation for IOS < 11.0 and OSX
 < 10.13.

Prior OS versions are missing the 10bpp formats.
---
 libavcodec/videotoolboxenc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Jan Ekström May 21, 2019, 11:41 p.m.
Hi,

On Tue, May 14, 2019 at 4:16 PM Thilo Borgmann <thilo.borgmann@mail.de> wrote:
>
> $Subject
>
> Tested compilation only, sanity test actually using it appreciated.
>

Thanks for the patch. To be completely fair, this is not to fix
compilation for specific target systems, but rather to fix compilation
on older SDKs (building with newer SDKs you can still build aiming for
macOS starting from 10.9, for example).

I didn't notice a patch landed on the encoder side that utilized the
defines without further checking/ifdefs. Too bad. I think I
specifically didn't yet merge the full/limited range patch on the
decoder side due to related reasons.

I did notice that VLC just re-defined these enum values themselves to
stop needing to have ifdefs depending on which SDK is being utilized
(https://github.com/videolan/vlc/commit/1b7e1c4bfcda375e2d4e657135aeaf3732e44af2#diff-a11cdb805d111956af60619d7dfa022bR735).
I wonder if we should have a helper header that would re-define these
enum values with their name. That way the code would look correct, and
the resulting binary has the same features independent of the SDK it
had been built under.

What would be the opinion of people on a solution like this?

Best regards,
Jan
Thilo Borgmann May 22, 2019, 12:27 p.m.
Am 22.05.19 um 01:41 schrieb Jan Ekström:
> Hi,
> 
> On Tue, May 14, 2019 at 4:16 PM Thilo Borgmann <thilo.borgmann@mail.de> wrote:
>>
>> $Subject
>>
>> Tested compilation only, sanity test actually using it appreciated.
>>
> 
> Thanks for the patch. To be completely fair, this is not to fix
> compilation for specific target systems, but rather to fix compilation
> on older SDKs (building with newer SDKs you can still build aiming for
> macOS starting from 10.9, for example).
> 
> I didn't notice a patch landed on the encoder side that utilized the
> defines without further checking/ifdefs. Too bad. I think I
> specifically didn't yet merge the full/limited range patch on the
> decoder side due to related reasons.
> 
> I did notice that VLC just re-defined these enum values themselves to
> stop needing to have ifdefs depending on which SDK is being utilized
> (https://github.com/videolan/vlc/commit/1b7e1c4bfcda375e2d4e657135aeaf3732e44af2#diff-a11cdb805d111956af60619d7dfa022bR735).
> I wonder if we should have a helper header that would re-define these
> enum values with their name. That way the code would look correct, and
> the resulting binary has the same features independent of the SDK it
> had been built under.
> 
> What would be the opinion of people on a solution like this?

Tested with a local definition of the symbols (like a would be header would do).
Seems to work for building with -macosx-version-min=XXX.

Also checked with VLC, they do these checks via thinks like:

#ifndef MAC_OS_X_VERSION_10_13
	...
#endif

Wich might be a better alternative to what I suggested.

Thus I would be fine with a helping header.

-Thilo
Thilo Borgmann May 24, 2019, 10:34 p.m.
Am 22.05.19 um 14:27 schrieb Thilo Borgmann:
> Am 22.05.19 um 01:41 schrieb Jan Ekström:
>> Hi,
>>
>> On Tue, May 14, 2019 at 4:16 PM Thilo Borgmann <thilo.borgmann@mail.de> wrote:
>>>
>>> $Subject
>>>
>>> Tested compilation only, sanity test actually using it appreciated.
>>>
>>
>> Thanks for the patch. To be completely fair, this is not to fix
>> compilation for specific target systems, but rather to fix compilation
>> on older SDKs (building with newer SDKs you can still build aiming for
>> macOS starting from 10.9, for example).
>>
>> I didn't notice a patch landed on the encoder side that utilized the
>> defines without further checking/ifdefs. Too bad. I think I
>> specifically didn't yet merge the full/limited range patch on the
>> decoder side due to related reasons.
>>
>> I did notice that VLC just re-defined these enum values themselves to
>> stop needing to have ifdefs depending on which SDK is being utilized
>> (https://github.com/videolan/vlc/commit/1b7e1c4bfcda375e2d4e657135aeaf3732e44af2#diff-a11cdb805d111956af60619d7dfa022bR735).
>> I wonder if we should have a helper header that would re-define these
>> enum values with their name. That way the code would look correct, and
>> the resulting binary has the same features independent of the SDK it
>> had been built under.
>>
>> What would be the opinion of people on a solution like this?
> 
> Tested with a local definition of the symbols (like a would be header would do).
> Seems to work for building with -macosx-version-min=XXX.
> 
> Also checked with VLC, they do these checks via thinks like:
> 
> #ifndef MAC_OS_X_VERSION_10_13
> 	...
> #endif
> 
> Wich might be a better alternative to what I suggested.
> 
> Thus I would be fine with a helping header.

If nobody else cares, should I try to come up with something like this? Or do you want to?

-Thilo
Richard Kern May 30, 2019, 6:14 p.m.
On Fri, May 24, 2019 at 6:34 PM Thilo Borgmann <thilo.borgmann@mail.de>
wrote:

> Am 22.05.19 um 14:27 schrieb Thilo Borgmann:
> > Am 22.05.19 um 01:41 schrieb Jan Ekström:
> >> Hi,
> >>
> >> On Tue, May 14, 2019 at 4:16 PM Thilo Borgmann <thilo.borgmann@mail.de>
> wrote:
> >>>
> >>> $Subject
> >>>
> >>> Tested compilation only, sanity test actually using it appreciated.
> >>>
> >>
> >> Thanks for the patch. To be completely fair, this is not to fix
> >> compilation for specific target systems, but rather to fix compilation
> >> on older SDKs (building with newer SDKs you can still build aiming for
> >> macOS starting from 10.9, for example).
> >>
> >> I didn't notice a patch landed on the encoder side that utilized the
> >> defines without further checking/ifdefs. Too bad. I think I
> >> specifically didn't yet merge the full/limited range patch on the
> >> decoder side due to related reasons.
> >>
> >> I did notice that VLC just re-defined these enum values themselves to
> >> stop needing to have ifdefs depending on which SDK is being utilized
> >> (
> https://github.com/videolan/vlc/commit/1b7e1c4bfcda375e2d4e657135aeaf3732e44af2#diff-a11cdb805d111956af60619d7dfa022bR735
> ).
> >> I wonder if we should have a helper header that would re-define these
> >> enum values with their name. That way the code would look correct, and
> >> the resulting binary has the same features independent of the SDK it
> >> had been built under.
> >>
> >> What would be the opinion of people on a solution like this?
> >
> > Tested with a local definition of the symbols (like a would be header
> would do).
> > Seems to work for building with -macosx-version-min=XXX.
> >
> > Also checked with VLC, they do these checks via thinks like:
> >
> > #ifndef MAC_OS_X_VERSION_10_13
> >       ...
> > #endif
> >
> > Wich might be a better alternative to what I suggested.
> >
> > Thus I would be fine with a helping header.
>
> If nobody else cares, should I try to come up with something like this? Or
> do you want to?
>

If you're willing to maintain the header as new values are added, it will
make it easier to read the code.


>
> -Thilo
> _______________________________________________
> 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".

Patch hide | download patch | download mbox

diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
index 3665581..24e0b62 100644
--- a/libavcodec/videotoolboxenc.c
+++ b/libavcodec/videotoolboxenc.c
@@ -762,12 +762,17 @@  static int get_cv_pixel_format(AVCodecContext* avctx,
         *av_pixel_format = range == AVCOL_RANGE_JPEG ?
                                         kCVPixelFormatType_420YpCbCr8PlanarFullRange :
                                         kCVPixelFormatType_420YpCbCr8Planar;
-    } else if (fmt == AV_PIX_FMT_P010LE) {
+    }
+#if ((!TARGET_OS_IPHONE && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || \
+     (TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000))
+    else if (fmt == AV_PIX_FMT_P010LE) {
         *av_pixel_format = range == AVCOL_RANGE_JPEG ?
                                         kCVPixelFormatType_420YpCbCr10BiPlanarFullRange :
                                         kCVPixelFormatType_420YpCbCr10BiPlanarVideoRange;
         *av_pixel_format = kCVPixelFormatType_420YpCbCr10BiPlanarVideoRange;
-    } else {
+    }
+#endif
+    else {
         return AVERROR(EINVAL);
     }
 
@@ -1991,6 +1996,8 @@  static int get_cv_pixel_info(
         strides[2] = frame ? frame->linesize[2] : (avctx->width + 1) / 2;
         break;
 
+#if ((!TARGET_OS_IPHONE && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || \
+     (TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000))
     case AV_PIX_FMT_P010LE:
         *plane_count = 2;
         widths[0] = avctx->width;
@@ -2001,6 +2008,7 @@  static int get_cv_pixel_info(
         heights[1] = (avctx->height + 1) / 2;
         strides[1] = frame ? frame->linesize[1] : ((avctx->width + 1) / 2 + 63) & -64;
         break;
+#endif
 
     default:
         av_log(
@@ -2488,7 +2496,10 @@  static const enum AVPixelFormat hevc_pix_fmts[] = {
     AV_PIX_FMT_VIDEOTOOLBOX,
     AV_PIX_FMT_NV12,
     AV_PIX_FMT_YUV420P,
+#if ((!TARGET_OS_IPHONE && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || \
+     (TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000))
     AV_PIX_FMT_P010LE,
+#endif
     AV_PIX_FMT_NONE
 };