Hi,
On Sat, Jun 24, 2017 at 3:50 PM, Ashish Singh <ashk43...@gmail.com> wrote:
> +typedef struct VMAFContext {
>
[..]
> + double curr_vmaf_score;
>
Unused.
> + uint64_t nb_frames;
>
Unused.
> + pthread_attr_t attr;
>
This is essentially unused, so might just as well pass NULL into
pthread_create().
> + int psnr;
> + int ssim;
> + int ms_ssim;
>
Hm... I see you're trying to replicate the commandline arguments here, but
we already have psnr/ssim filters by themselves. I'm not sure we need those
options here. (If others think we should keep them or you have a strong
opinion on keeping it, we can keep it.)
> + FILE *stats_file;
> + char *stats_file_str;
>
Unused.
> + int stats_version;
>
Unused, and also unneeded. stats_version was added to some other filters so
we could update the stats_file format without breaking backwards
compatibility for applications expecting the old log format. This filter
has no "old" or "new" log format yet, so this isn't needed.
> + int stats_header_written;
> + int stats_add_max;
> + int nb_components;
>
Unused.
> +static const AVOption vmaf_options[] = {
>
[..]
> + {"stats_file", "Set file where to store per-frame difference
> information", OFFSET(stats_file_str), AV_OPT_TYPE_STRING, {.str=NULL}, 0,
> 0, FLAGS },
> + {"f", "Set file where to store per-frame difference
> information", OFFSET(stats_file_str), AV_OPT_TYPE_STRING, {.str=NULL}, 0,
> 0, FLAGS },
> + {"stats_version", "Set the format version for the stats file.",
> OFFSET(stats_version), AV_OPT_TYPE_INT, {.i64=1}, 1, 2,
> FLAGS },
> + {"output_max", "Add raw stats (max values) to the output log.",
> OFFSET(stats_add_max), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
>
Unused.
> + {"model_path", "Set the model to be used for computing vmaf.",
> OFFSET(model_path), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, FLAGS},
>
I would suggest to add a non-NULL default model path here (instead of {
.str = NULL }). Maybe the default path can be detected by configure?
> + {"psnr", "Enables computing psnr along with vmaf.",
> OFFSET(psnr), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
> + {"ssim", "Enables computing ssim along with vmaf.",
> OFFSET(ssim), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
> + {"ms_ssim", "Enables computing ms-ssim along with vmaf.",
> OFFSET(ms_ssim), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS},
>
See related comment above.
> +static int read_frame_8bit(float *ref_data, float *main_data, int stride,
> double *score, void *ctx){
> +
>
{ Should go on new line for function declarations.
> + VMAFContext *s = (VMAFContext *)ctx;
>
.. = (VMAFContext *) ctx; << note space between ")" and "ctx"
> + if (s->eof == 1) {
> + s->eof++;
> + }
> + else if (s->eof == 2) {
> + return s->eof;
> + }
>
Umm, this isn't what I meant with my comment earlier. There's a race
condition here where s->eof is written to by a different thread but you
read it outside the critical section within s->lock protection:
> + pthread_mutex_lock(&s->lock);
> +
> + while (s->frame_set == 0) {
> + pthread_cond_wait(&s->cond, &s->lock);
> + }
>
So at this point, you have two variables (s->eof and s->frame_set) both
accessible from s->lock. That's fine, but the problem is that you're not
checking whether s->frame_set is 1 before exiting based on s->eof values.
My suggestion is something like this:
pthread_mutex_lock(&s->lock);
while (!s->frame_set && !s->eof) {
pthread_cond_wait(&s->cond, &s->lock);
}
if (s->frame_set) {
.. copy data into vmaf buffers
}
int res = !s->frame_set;
s->frame_set = 0;
pthread_cond_signal(&s->cond);
pthread_mutex_unlock(&s->lock);
return ret;
+static int read_frame_10bit(float *ref_data, float *main_data, int stride,
> double *score, void *ctx){
>
I'd also recommend to tempalte the read_frame_8/10bit() functions, that
makes maintenance easier because it reduces source code size. But that can
be done later...
+static void compute_vmaf_score(VMAFContext *s)
> +{
> + void *func;
>
int (*read_frame)(int arg1, int arg2);
> + if (strcmp(s->format, "yuv420p") || strcmp(s->format, "yuv422p") ||
> strcmp(s->format, "yuv444p")) {
> + func = read_frame_8bit;
>
read_frame = read_frame_8bit;
> + }
> + else {
>
"} else {" all on same line.
+ func = read_frame_10bit;
>
read_frame = read_frame_10bit;
> +static av_cold int init(AVFilterContext *ctx)
> +{
>
[..]
> + if (s->stats_file_str) {
> + if (s->stats_version < 2 && s->stats_add_max) {
> + av_log(ctx, AV_LOG_ERROR,
> + "stats_add_max was specified but stats_version < 2.\n"
> );
> + return AVERROR(EINVAL);
> + }
> + if (!strcmp(s->stats_file_str, "-")) {
> + s->stats_file = stdout;
> + } else {
> + s->stats_file = fopen(s->stats_file_str, "w");
> + if (!s->stats_file) {
> + int err = AVERROR(errno);
> + char buf[128];
> + av_strerror(err, buf, sizeof(buf));
> + av_log(ctx, AV_LOG_ERROR, "Could not open stats file %s:
> %s\n",
> + s->stats_file_str, buf);
> + return err;
> + }
> + }
> + }
>
I think ATM all this code can be removed.
> +static av_cold void uninit(AVFilterContext *ctx)
>
[..]
> + if (s->stats_file && s->stats_file != stdout)
> + fclose(s->stats_file);
>
Can be removed.
> + av_log(ctx, AV_LOG_INFO, "VMAF score: %f\n",s->vmaf_score);
> +
> +}
>
Remove empty line at the end.
Don't forget to pthread_mutex/cond_destroy() the cond and lock variables.
Ronald
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
