[FFmpeg-devel] [GSOC] [PATCH] SRCNN filter

[Pedro Arthur]

2018-05-03 17:58 GMT-03:00 Michael Niedermayer <michael at niedermayer.cc>:
> On Thu, May 03, 2018 at 03:17:11PM -0300, Pedro Arthur wrote:
>> 2018-04-10 14:16 GMT-03:00 Sergey Lavrushkin <dualfal at gmail.com>:
>> > 2018-03-29 3:55 GMT+03:00 Michael Niedermayer <michael at niedermayer.cc>:
>> >
>> >> On Wed, Mar 28, 2018 at 11:17:40AM +0300, Sergey Lavrushkin wrote:
>> >> > > [...]
>> >> > > > +#define OFFSET(x) offsetof(SRCNNContext, x)
>> >> > > > +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM | AV_OPT_FLAG_VIDEO_PARAM
>> >> > > > +static const AVOption srcnn_options[] = {
>> >> > > > +    { "config_file", "path to configuration file with network
>> >> > > parameters", OFFSET(config_file_path), AV_OPT_TYPE_STRING,
>> >> {.str=NULL}, 0,
>> >> > > 0, FLAGS },
>> >> > > > +    { NULL }
>> >> > > > +};
>> >> > > > +
>> >> > > > +AVFILTER_DEFINE_CLASS(srcnn);
>> >> > > > +
>> >> > > > +#define CHECK_FILE(file)    if (ferror(file) || feof(file)){ \
>> >> > > > +                                av_log(context, AV_LOG_ERROR, "error
>> >> > > reading configuration file\n");\
>> >> > > > +                                fclose(file); \
>> >> > > > +                                return AVERROR(EIO); \
>> >> > > > +                            }
>> >> > > > +
>> >> > > > +#define CHECK_ALLOCATION(conv, file)    if
>> >> > > (allocate_and_read_convolution_data(&conv, file)){ \
>> >> > > > +                                            av_log(context,
>> >> > > AV_LOG_ERROR, "could not allocate memory for convolutions\n"); \
>> >> > > > +                                            fclose(file); \
>> >> > > > +                                            return AVERROR(ENOMEM);
>> >> \
>> >> > > > +                                        }
>> >> > > > +
>> >> > >
>> >> > > > +static int allocate_and_read_convolution_data(Convolution* conv,
>> >> FILE*
>> >> > > config_file)
>> >> > > > +{
>> >> > > > +    int32_t kernel_size = conv->output_channels * conv->size *
>> >> > > conv->size * conv->input_channels;
>> >> > > > +    conv->kernel = av_malloc(kernel_size * sizeof(double));
>> >> > > > +    if (!conv->kernel){
>> >> > > > +        return 1;
>> >> > >
>> >> > > this should return an AVERROR code for consistency with the rest of
>> >> > > the codebase
>> >> > >
>> >> >
>> >> > Ok.
>> >> >
>> >> >
>> >> > > > +    }
>> >> > >
>> >> > > > +    fread(conv->kernel, sizeof(double), kernel_size, config_file);
>> >> > >
>> >> > > directly reading data types is not portable, it would for example be
>> >> > > endian specific
>> >> > > and using avio for reading may be better, though fread is as far as iam
>> >> > > concerned also ok
>> >> > >
>> >> >
>> >> > Ok, I understand the problem, but I have not really worked with it
>> >> before,
>> >> > so I need an advice of how to properly fix it. If I understand correctly,
>> >> > for
>> >>
>> >> > int32_t I need to check endianness and reverse bytes if necessary. But
>> >> with
>> >>
>> >> see avio_rb32()
>> >> might not be as fast as reading a whole array and byteswaping though.
>> >> Not sure it matters
>> >>
>> >>
>> >> > doubles it is more complicated. Should I write a IEEE 754 converter from
>> >> > binary
>> >> > to double or maybe I can somehow check IEEE 754 doubles support and
>> >> > depending
>> >> > on it either stick to the default network weights, or just read bytes and
>> >> > check
>> >> > endianness, if IEEE 754 doubles are supported? Or maybe avio provide some
>> >> > utility to deal with this problem?
>> >>
>> >> something like this should work:
>> >> av_int2float(avio_rb32(pb));
>> >> av_int2double(avio_rb64(pb));
>> >>
>> >> >
>> >> >
>> >> > > [...]
>> >> > > > +/**
>> >> > > > + * @file
>> >> > > > + * Default cnn weights for x2 upsampling with srcnn filter.
>> >> > > > + */
>> >> > > > +
>> >> > > > +/// First convolution kernel
>> >> > >
>> >> > > > +static double conv1_kernel[] = {
>> >> > >
>> >> > > static data should be also const, otherwise it may be changed and could
>> >> > > cause
>> >> > > thread saftey issues
>> >> > >
>> >> >
>> >> > Ok, I just wanted to not allocate additional memory in case of using
>> >> > default weights.
>> >>
>> >> well, there can be more than one instance of a filter running at the same
>> >> time
>> >> so static variables that are actually changing will affect the other
>> >> filter instance
>> >>
>> >
>> > Hi,
>> >
>> > Here is the updated patch with fixed issues.
>> > Sorry for the late reply, I was busy doing things related to my university
>> > activities.
>> >
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel at ffmpeg.org
>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >
>>
>> I think all concerns were addressed, can the patch be pushed?
>
> sure you can push it if it looks ok to you.
> please make sure fate-source passes (i think it fails with the patch)
Fixed and pushed.

>
> thanks
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Into a blind darkness they enter who follow after the Ignorance,
> they as if into a greater darkness enter who devote themselves
> to the Knowledge alone. -- Isha Upanishad
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>