[FFmpeg-devel] [PATCH] [PATCH] avdevice/avfoundation: add scaleFactor attribute for avfoundation

[Thilo Borgmann]

Am 07.08.17 um 01:31 schrieb sharpbai:
> From: sharpbai <tian.bai at duobei.com>
> 
> feature: add scaleFactor attribute for avfoundation
> added by: siyuan.wang at duobei.com
> added by: yiren.li at duobei.com
> ---
>  doc/indevs.texi            | 22 ++++++++++++++++++++++
>  libavdevice/avfoundation.m |  6 ++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/doc/indevs.texi b/doc/indevs.texi
> index 09e3321..1ba71d7 100644
> --- a/doc/indevs.texi
> +++ b/doc/indevs.texi
> @@ -139,6 +139,13 @@ Capture the mouse pointer. Default is 0.
>  @item -capture_mouse_clicks
>  Capture the screen mouse clicks. Default is 0.
>  
> + at item -scale_factor
> +Scale factor for capture the screen. Set this property to scale the buffers
"Scale factor during screen capturing. Set this property to scale the screens by a given factor." 


> +by a given factor. For example capturing a retina screen which resolution 2880x1800 
Trailing whitespace.


> +with a scale_factor of 0.5 (or 0.05) produces video buffers at 1440x900 (or 144x90).
The example part should cover that so this can be removed.


> +This is useful for reducing captured file size and increasing performance 
> +in capturing screen. 
Can also be removed.


> Default is 1.0 (no scaling).
> +
>  @end table
>  
>  @subsection Examples
> @@ -169,6 +176,21 @@ Record video from the system default video device using the pixel format bgr0 an
>  $ ffmpeg -f avfoundation -pixel_format bgr0 -i "default:none" out.avi
>  @end example
>  
> + at item
> +Capture video from the first screen using the pixel format bgr0 and scaling in half size into out.avi.
> +First command use the avfoundation device command to enumerate all the available input devices including
> +screens ready to be captured:
> + at example
> +$ ffmpeg -f avfoundation -list_devices true -i ""
> + at end example
> +Once you've figured out the device index corresponding to the screen to be captured use, run the second
> +command with the correct screen device index to execute screen capture. 
The documentation already covers the block above so this should be removed.


> For example on my mac the index
> +of "Screen Capture 0" is "1", I should replace @code{-i "<screen_device_index>"} with @code{-i "1"} in the second command.
> + at example
> +$ ffmpeg -f avfoundation -pixel_format bgr0 -scale_factor 0.5 -i "<screen_device_index>" out.avi
> + at end example
> +
> +
Remove things like "on my mac" and "I should...".
See the other examples for reference.


>  @end itemize
>  
>  @section bktr
> diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
> index e2ddf47..1196cf3 100644
> --- a/libavdevice/avfoundation.m
> +++ b/libavdevice/avfoundation.m
> @@ -96,6 +96,7 @@ typedef struct
>  
>      int             capture_cursor;
>      int             capture_mouse_clicks;
> +    float           scale_factor;
>  
>      int             list_devices;
>      int             video_device_index;
> @@ -735,6 +736,10 @@ static int avf_read_header(AVFormatContext *s)
>                  capture_screen_input.minFrameDuration = CMTimeMake(ctx->framerate.den, ctx->framerate.num);
>              }
>  
> +            if (ctx->scale_factor > 0.0) {
> +                capture_screen_input.scaleFactor = ctx->scale_factor;
> +            }
> +
>  #if !TARGET_OS_IPHONE && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080
>              if (ctx->capture_cursor) {
>                  capture_screen_input.capturesCursor = YES;
> @@ -1025,6 +1030,7 @@ static const AVOption options[] = {
>      { "video_size", "set video size", offsetof(AVFContext, width), AV_OPT_TYPE_IMAGE_SIZE, {.str = NULL}, 0, 0, AV_OPT_FLAG_DECODING_PARAM },
>      { "capture_cursor", "capture the screen cursor", offsetof(AVFContext, capture_cursor), AV_OPT_TYPE_INT, {.i64=0}, 0, 1, AV_OPT_FLAG_DECODING_PARAM },
>      { "capture_mouse_clicks", "capture the screen mouse clicks", offsetof(AVFContext, capture_mouse_clicks), AV_OPT_TYPE_INT, {.i64=0}, 0, 1, AV_OPT_FLAG_DECODING_PARAM },
> +    { "scale_factor", "scale screen factor range", offsetof(AVFContext, scale_factor), AV_OPT_TYPE_FLOAT, {.dbl=1.0}, 0.0, 1.0, AV_OPT_FLAG_DECODING_PARAM },
This is still a value and not a range.


>  
>      { NULL },
>  };
> 

Instead of having the others find your changes, you should comment on any raised questions rather than just adapting your code wherever things are non trivial.

Your patch misses to cover the scale factor in case of a device name string given by the user.

Do you have an idea why only 0.0 < scale_factor <= 1.0 is working correctly?
What happens for scale_factor == 0.0?
Although Apple's reference says it works for scale_factor > 1.0, it should be documented (at least in the code) that it seems not to actually do anything for scale_factor > 1.0.

-Thilo