[FFmpeg-devel] [PATCH v2 1/3] avfilter: add v360 filter

[Thilo Borgmann]

Am 14.08.19 um 16:09 schrieb Paul B Mahol:
> On Wed, Aug 14, 2019 at 4:06 PM Thilo Borgmann <thilo.borgmann at mail.de>
> wrote:
> 
>> Am 14.08.19 um 15:14 schrieb Eugene:
>>> On 14.08.2019 13:06, Paul B Mahol wrote:
>>>
>>>> On Wed, Aug 14, 2019 at 11:56 AM Thilo Borgmann <thilo.borgmann at mail.de
>>>
>>>> wrote:
>>>>
>>>>> [...]
>>>>>
>>>>>>>> +static int config_output(AVFilterLink *outlink)
>>>>>>>> +{
>>>>>>>> +    AVFilterContext *ctx = outlink->src;
>>>>>>>> +    AVFilterLink *inlink = ctx->inputs[0];
>>>>>>>> +    V360Context *s = ctx->priv;
>>>>>>>> +    const AVPixFmtDescriptor *desc =
>>>>>>>> av_pix_fmt_desc_get(inlink->format);
>>>>>>>> +    const int depth = desc->comp[0].depth;
>>>>>>>> +    float remap_data_size = 0.f;
>>>>>>>> +    int sizeof_remap;
>>>>>>>> +    int err;
>>>>>>>> +    int p, h, w;
>>>>>>>> +    float hf, wf;
>>>>>>>> +    float mirror_modifier[3];
>>>>>>>> +    void (*in_transform)(const V360Context *s,
>>>>>>>> +                         const float *vec, int width, int height,
>>>>>>>> +                         uint16_t us[4][4], uint16_t vs[4][4],
>> float
>>>>>>>> *du, float *dv);
>>>>>>>> +    void (*out_transform)(const V360Context *s,
>>>>>>>> +                          int i, int j, int width, int height,
>>>>>>>> +                          float *vec);
>>>>>>>> +    void (*calculate_kernel)(float du, float dv, int shift, const
>>>>>>> XYRemap4
>>>>>>>> *r_tmp, void *r);
>>>>>>>> +    float rot_mat[3][3];
>>>>>>>> +
>>>>>>>> +    switch (s->interp) {
>>>>>>>> +    case NEAREST:
>>>>>>>> +        calculate_kernel = nearest_kernel;
>>>>>>>> +        s->remap_slice = depth <= 8 ? remap1_8bit_slice :
>>>>>>>> remap1_16bit_slice;
>>>>>>>> +        sizeof_remap = sizeof(XYRemap1);
>>>>>>>> +        break;
>>>>>>>> +    case BILINEAR:
>>>>>>>> +        calculate_kernel = bilinear_kernel;
>>>>>>>> +        s->remap_slice = depth <= 8 ? remap2_8bit_slice :
>>>>>>>> remap2_16bit_slice;
>>>>>>>> +        sizeof_remap = sizeof(XYRemap2);
>>>>>>>> +        break;
>>>>>>>> +    case BICUBIC:
>>>>>>>> +        calculate_kernel = bicubic_kernel;
>>>>>>>> +        s->remap_slice = depth <= 8 ? remap4_8bit_slice :
>>>>>>>> remap4_16bit_slice;
>>>>>>>> +        sizeof_remap = sizeof(XYRemap4);
>>>>>>>> +        break;
>>>>>>>> +    case LANCZOS:
>>>>>>>> +        calculate_kernel = lanczos_kernel;
>>>>>>>> +        s->remap_slice = depth <= 8 ? remap4_8bit_slice :
>>>>>>>> remap4_16bit_slice;
>>>>>>>> +        sizeof_remap = sizeof(XYRemap4);
>>>>>>>> +        break;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    switch (s->in) {
>>>>>>>> +    case EQUIRECTANGULAR:
>>>>>>>> +        in_transform = xyz_to_equirect;
>>>>>>>> +        err = 0;
>>>>>>>> +        wf = inlink->w;
>>>>>>>> +        hf = inlink->h;
>>>>>>>> +        break;
>>>>>>>> +    case CUBEMAP_3_2:
>>>>>>>> +        in_transform = xyz_to_cube3x2;
>>>>>>>> +        err = prepare_cube_in(ctx);
>>>>>>>> +        wf = inlink->w / 3.f * 4.f;
>>>>>>>> +        hf = inlink->h;
>>>>>>>> +        break;
>>>>>>>> +    case CUBEMAP_6_1:
>>>>>>>> +        in_transform = xyz_to_cube6x1;
>>>>>>>> +        err = prepare_cube_in(ctx);
>>>>>>>> +        wf = inlink->w / 3.f * 2.f;
>>>>>>>> +        hf = inlink->h * 2.f;
>>>>>>>> +        break;
>>>>>>>> +    case EQUIANGULAR:
>>>>>>>> +        in_transform = xyz_to_eac;
>>>>>>>> +        err = prepare_eac_in(ctx);
>>>>>>>> +        wf = inlink->w;
>>>>>>>> +        hf = inlink->h / 9.f * 8.f;
>>>>>>>> +        break;
>>>>>>>> +    case FLAT:
>>>>>>>> +        av_log(ctx, AV_LOG_ERROR, "Flat format is not accepted as
>>>>>>>> input.\n");
>>>>>>>> +        return AVERROR(EINVAL);
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    if (err != 0) {
>>>>>>>> +        return err;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    switch (s->out) {
>>>>>>>> +    case EQUIRECTANGULAR:
>>>>>>>> +        out_transform = equirect_to_xyz;
>>>>>>>> +        err = 0;
>>>>>>>> +        w = roundf(wf);
>>>>>>>> +        h = roundf(hf);
>>>>>>>> +        break;
>>>>>>>> +    case CUBEMAP_3_2:
>>>>>>>> +        out_transform = cube3x2_to_xyz;
>>>>>>>> +        err = prepare_cube_out(ctx);
>>>>>>>> +        w = roundf(wf / 4.f * 3.f);
>>>>>>>> +        h = roundf(hf);
>>>>>>>> +        break;
>>>>>>>> +    case CUBEMAP_6_1:
>>>>>>>> +        out_transform = cube6x1_to_xyz;
>>>>>>>> +        err = prepare_cube_out(ctx);
>>>>>>>> +        w = roundf(wf / 2.f * 3.f);
>>>>>>>> +        h = roundf(hf / 2.f);
>>>>>>>> +        break;
>>>>>>>> +    case EQUIANGULAR:
>>>>>>>> +        out_transform = eac_to_xyz;
>>>>>>>> +        err = prepare_eac_out(ctx);
>>>>>>>> +        w = roundf(wf);
>>>>>>>> +        h = roundf(hf / 8.f * 9.f);
>>>>>>>> +        break;
>>>>>>>> +    case FLAT:
>>>>>>>> +        out_transform = flat_to_xyz;
>>>>>>>> +        err = prepare_flat_out(ctx);
>>>>>>>> +        w = roundf(wf * s->flat_range[0] / s->flat_range[1] / 2.f);
>>>>>>>> +        h = roundf(hf);
>>>>>>>> +        break;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    if (err != 0) {
>>>>>>>> +        return err;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    if (s->width > 0 && s->height > 0) {
>>>>>>>> +        w = s->width;
>>>>>>>> +        h = s->height;
>>>>>>>> +    }
>>>>>>> If s->width/height are checked, should handle the case of no ture,
>>>>>>> Else w/h may be used but not initialized.
>>>>>>>
>>>>>> Please try more hard to write english. I do not understand what is
>> typed
>>>>>> above.
>>>>> If w and h don't have initial values at declaration. So they might
>> never
>>>>> get set if that last if statement evaluates to false.
>>>>>
>>>>> So he suggests to add an else case like
>>>>>
>>>>> if (s->width > 0 && s->height > 0) {
>>>>>   w = s->width;
>>>>>   h = s->height;
>>>>> } else {
>>>>>   w = 0;
>>>>>   h = 0;
>>>>> }
>>>>>
>>>>> or whatever values might make sense.
>>>>>
>>>> else case should have assert or return AVERROR_BUG.
>>>
>>> This "if" branch overrides default values with user values. Default
>> values are calculated just above this code based on input/output formats.
>>> So w and h do get initialized in any case.
>>
>> Hmm... what happens if (s->out != any case you have in the switch) and
>> (s->width < 1 || s->height < 1)
>>
>> Maybe I misread, actually I just clarified what Li Zhong said.
>>
> 
> It is covered, one might add simply assert to make sure sloppy developer
> does not leave above code unchecked.

Think so, too. Please go for an assert or AVERROR else branch like Paul suggested, Eugene.

-Thilo