[FFmpeg-devel,2/3] decklink: new option 'format' to set video format by fourCC

---
 libavdevice/decklink_common.cpp | 22 ++++++++++++++++++----
 libavdevice/decklink_common_c.h |  1 +
 libavdevice/decklink_dec.cpp    |  2 +-
 libavdevice/decklink_dec_c.c    |  1 +
 4 files changed, 21 insertions(+), 5 deletions(-)

Le sextidi 26 ventôse, an CCXXV, Matthias Hunstock a écrit :
> ---
>  libavdevice/decklink_common.cpp | 22 ++++++++++++++++++----
>  libavdevice/decklink_common_c.h |  1 +
>  libavdevice/decklink_dec.cpp    |  2 +-
>  libavdevice/decklink_dec_c.c    |  1 +
>  4 files changed, 21 insertions(+), 5 deletions(-)

I would advise to use a name much more specific than "format".

Regards,

Am 16.03.2017 um 15:40 schrieb Nicolas George:
> I would advise to use a name much more specific than "format".

I did not find a comparable option for another device in libavdevice,
"format" was the initial suggestion of Marton. This is at least
consistent with the existing "list_formats" option.

Other suggestions:

  video_format
  video_mode
  video_standard
  signal_standard
  ...?

Matthias

Le sextidi 26 ventôse, an CCXXV, Matthias Hunstock a écrit :
> I did not find a comparable option for another device in libavdevice,
> "format" was the initial suggestion of Marton. This is at least
> consistent with the existing "list_formats" option.

I do not know what the option does, and therefore can not propose a
better name.

All I know is that we decided not to build an inconvenient
infrastructure to avoid the collision between global, format, codecs
options, at the cost of having to be careful when adding said options.
This is exactly one of these cases: "format" can be added as a global
option, as a ffmpeg.c option or anything else, because it is very
generic.

Regards,

On Thu, 16 Mar 2017, Nicolas George wrote:

> Le sextidi 26 ventôse, an CCXXV, Matthias Hunstock a écrit :
>> I did not find a comparable option for another device in libavdevice,
>> "format" was the initial suggestion of Marton. This is at least
>> consistent with the existing "list_formats" option.
>
> I do not know what the option does, and therefore can not propose a
> better name.

It selects video standard, field order and frame rate, these fourcc-s are 
defined in the BlackMagic SDK. Example: 'Hp50'  - HD 1080p50 format.

>
> All I know is that we decided not to build an inconvenient
> infrastructure to avoid the collision between global, format, codecs
> options, at the cost of having to be careful when adding said options.
> This is exactly one of these cases: "format" can be added as a global
> option, as a ffmpeg.c option or anything else, because it is very
> generic.

Then the next guy should worry about this who wants to add a "format" 
option :)

The argument you made is true for a lot of other generic strings, such as 
"video_format" or "standard", so if I really want to make sure I don't use 
generic option names, I guess I will end up with a prefixed option name, 
such as bm_format. Only a few codecs/demuxers/devices do that.

So overall, I am not sure it's worth losing the consistency (using a 
prefixed option name, where almost all other option names are 
non-prefixed), but I don't really mind either way if we use bm_format 
instead.

Regards,
Marton

Le sextidi 26 ventôse, an CCXXV, Marton Balint a écrit :
> It selects video standard, field order and frame rate, these fourcc-s are
> defined in the BlackMagic SDK. Example: 'Hp50'  - HD 1080p50 format.

Then "format_code", "standard_code", "standard_id", etc.

> The argument you made is true for a lot of other generic strings, such as
> "video_format" or "standard"

It is true for all strings, more or less. More for "format", already a
lot less for the one you suggest right here.

Regards,

On Thu, 16 Mar 2017, Matthias Hunstock wrote:

> ---
> libavdevice/decklink_common.cpp | 22 ++++++++++++++++++----
> libavdevice/decklink_common_c.h |  1 +
> libavdevice/decklink_dec.cpp    |  2 +-
> libavdevice/decklink_dec_c.c    |  1 +
> 4 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/libavdevice/decklink_common.cpp b/libavdevice/decklink_common.cpp
> index 131d186..548bf3b 100644
> --- a/libavdevice/decklink_common.cpp
> +++ b/libavdevice/decklink_common.cpp
> @@ -143,6 +143,18 @@ static DECKLINK_BOOL field_order_eq(enum AVFieldOrder field_order, BMDFieldDomin
>     return false;
> }
> 
> +static DECKLINK_BOOL fourcc_eq(char *fourcc_string, BMDDisplayMode fourcc)
> +{
> +    BMDDisplayMode fourcc_converted = 0;
> +    if (!fourcc_string) return false;
> +    if (!strcmp(fourcc_string, "pal") && fourcc == 0x70616C20) return true;
> +    if (strlen(fourcc_string) < 4) return false;
> +    fourcc_converted = ((uint8_t) fourcc_string[0] << 24) | ((uint8_t) fourcc_string[1] << 16) |
> +        ((uint8_t) fourcc_string[2] << 8) | ((uint8_t) fourcc_string[3]);
> +    if (fourcc_converted == fourcc) return true;
> +    return false;
> +}

I think I'd use a little different approach. Instead of a separate _eq 
function, I'd transfrom the fourcc string to an int32 by padding it with 4 
spaces, then use AV_RL32(padded_fourcc) to get an actual int32. I can now 
simply compare this to BMDDisplayMode.

> +
> int ff_decklink_set_format(AVFormatContext *avctx,
>                                int width, int height,
>                                int tb_num, int tb_den,
> @@ -157,8 +169,8 @@ int ff_decklink_set_format(AVFormatContext *avctx,
>     int i = 1;
>     HRESULT res;
> 
> -    av_log(avctx, AV_LOG_DEBUG, "Trying to find mode for frame size %dx%d, frame timing %d/%d, field order %d, direction %d, mode number %d\n",
> -        width, height, tb_num, tb_den, field_order, direction, num);
> +    av_log(avctx, AV_LOG_DEBUG, "Trying to find mode for frame size %dx%d, frame timing %d/%d, field order %d, direction %d, mode number %d, mode fourCC %s\n",
> +        width, height, tb_num, tb_den, field_order, direction, num, cctx->format);

cctx->format ? cctx->format : "(unset)"

>
>     if (ctx->duplex_mode) {
>         DECKLINK_BOOL duplex_supported = false;
> @@ -202,6 +214,7 @@ int ff_decklink_set_format(AVFormatContext *avctx,
>         int bmd_width  = mode->GetWidth();
>         int bmd_height = mode->GetHeight();
>         BMDFieldDominance bmd_field_dominance = mode->GetFieldDominance();
> +        BMDDisplayMode bmd_mode = mode->GetDisplayMode();
>
>         mode->GetFrameRate(&bmd_tb_num, &bmd_tb_den);
>         AVRational mode_tb = av_make_q(bmd_tb_num, bmd_tb_den);
> @@ -209,8 +222,9 @@ int ff_decklink_set_format(AVFormatContext *avctx,
>         if ((bmd_width == width &&
>              bmd_height == height &&
>              !av_cmp_q(mode_tb, target_tb) &&
> -             field_order_eq(field_order, bmd_field_dominance)) || i == num) {
> -            ctx->bmd_mode   = mode->GetDisplayMode();
> +             field_order_eq(field_order, bmd_field_dominance))
> +             || i == num || fourcc_eq(cctx->format, bmd_mode)) {
> +            ctx->bmd_mode   = bmd_mode;
>             ctx->bmd_width  = bmd_width;
>             ctx->bmd_height = bmd_height;
>             ctx->bmd_tb_den = bmd_tb_den;
> diff --git a/libavdevice/decklink_common_c.h b/libavdevice/decklink_common_c.h
> index d565631..161a48a 100644
> --- a/libavdevice/decklink_common_c.h
> +++ b/libavdevice/decklink_common_c.h
> @@ -47,6 +47,7 @@ struct decklink_cctx {
>     int audio_input;
>     int video_input;
>     int draw_bars;
> +    char *format;
> };
> 
> #endif /* AVDEVICE_DECKLINK_COMMON_C_H */
> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
> index 7df841b..5948888 100644
> --- a/libavdevice/decklink_dec.cpp
> +++ b/libavdevice/decklink_dec.cpp
> @@ -539,7 +539,7 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx)
>         goto error;
>     }
> 
> -    if (mode_num > 0) {
> +    if (mode_num > 0 || cctx->format) {
>         if (ff_decklink_set_format(avctx, DIRECTION_IN, mode_num) < 0) {
>             av_log(avctx, AV_LOG_ERROR, "Could not set mode %d for %s\n", mode_num, fname);
>             ret = AVERROR(EIO);
> diff --git a/libavdevice/decklink_dec_c.c b/libavdevice/decklink_dec_c.c
> index 31818d2..07da7cc 100644
> --- a/libavdevice/decklink_dec_c.c
> +++ b/libavdevice/decklink_dec_c.c
> @@ -31,6 +31,7 @@
> static const AVOption options[] = {
>     { "list_devices", "list available devices"  , OFFSET(list_devices), AV_OPT_TYPE_INT   , { .i64 = 0   }, 0, 1, DEC },
>     { "list_formats", "list supported formats"  , OFFSET(list_formats), AV_OPT_TYPE_INT   , { .i64 = 0   }, 0, 1, DEC },
> +    { "format",       "set format by fourcc"    , OFFSET(format),       AV_OPT_TYPE_STRING, { .str = NULL}, 3, 4, DEC },
>     { "bm_v210",      "v210 10 bit per channel" , OFFSET(v210),         AV_OPT_TYPE_INT   , { .i64 = 0   }, 0, 1, DEC },
>     { "teletext_lines", "teletext lines bitmask", OFFSET(teletext_lines), AV_OPT_TYPE_INT64, { .i64 = 0   }, 0, 0x7ffffffffLL, DEC, "teletext_lines"},
>     { "standard",     NULL,                                           0,  AV_OPT_TYPE_CONST, { .i64 = 0x7fff9fffeLL}, 0, 0,    DEC, "teletext_lines"},
> -- 
> 2.1.4
>

Regards,
Marton

2017-03-16 21:54 GMT+01:00 Marton Balint <cus@passwd.hu>:
>
> On Thu, 16 Mar 2017, Nicolas George wrote:
>
>> Le sextidi 26 ventôse, an CCXXV, Matthias Hunstock a écrit :
>>>
>>> I did not find a comparable option for another device in libavdevice,
>>> "format" was the initial suggestion of Marton. This is at least
>>> consistent with the existing "list_formats" option.
>>
>>
>> I do not know what the option does, and therefore can not propose a
>> better name.
>
>
> It selects video standard, field order and frame rate, these fourcc-s are
> defined in the BlackMagic SDK. Example: 'Hp50'  - HD 1080p50 format.

I believe our v4l2 device calls the option "input_format".

Carl Eugen

On Thursday, March 16, 2017 3:54:39 PM CDT Marton Balint wrote:
> On Thu, 16 Mar 2017, Nicolas George wrote:
> > Le sextidi 26 ventôse, an CCXXV, Matthias Hunstock a écrit :
> >> I did not find a comparable option for another device in libavdevice,
> >> "format" was the initial suggestion of Marton. This is at least
> >> consistent with the existing "list_formats" option.
> > 
> > I do not know what the option does, and therefore can not propose a
> > better name.
> 
> It selects video standard, field order and frame rate, these fourcc-s are
> defined in the BlackMagic SDK. Example: 'Hp50'  - HD 1080p50 format.
> 
> > All I know is that we decided not to build an inconvenient
> > infrastructure to avoid the collision between global, format, codecs
> > options, at the cost of having to be careful when adding said options.
> > This is exactly one of these cases: "format" can be added as a global
> > option, as a ffmpeg.c option or anything else, because it is very
> > generic.
> 

How about “scan_format”, to be act as a collection of sub-elements 
“scan_width”, “scan_rate” and "scan_type"?