Conversation
6c4a462 to
ab83efc
Compare
| {"n_subsample", "Set interval for frame subsampling used when computing vmaf.", OFFSET(n_subsample), AV_OPT_TYPE_INT, {.i64=1}, 1, UINT_MAX, FLAGS}, | ||
| {"model", "Set the model to be used for computing vmaf.", OFFSET(model_cfg), AV_OPT_TYPE_STRING, {.str="version=vmaf_v0.6.1"}, 0, 1, FLAGS}, | ||
| {"feature", "Set the feature to be used for computing vmaf.", OFFSET(feature_cfg), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, FLAGS}, | ||
| {"metadata_handler", "Set the feature to be propagated as metadata.", OFFSET(metadata_feature_cfg), AV_OPT_TYPE_STRING, {.str="name=vmaf"}, 0, 1, FLAGS}, |
There was a problem hiding this comment.
I think it would be better to have a bool option to enable metadata writing for all features/models. If you do it in the way you've written, only one feature can be written as metadata. If you do it in the way I've suggested, when the option is set, you can register metadata callbacks for everything.
There was a problem hiding this comment.
individual feature names as well as model names should both be available during init.
There was a problem hiding this comment.
Actually users can give metadata handlers like this libvmaf='n_threads=30:feature=name=psnr|name=ciede:metadata_handler=name=vmaf|name=psnr_y|name=ciede2000' But obviously this makes hard to count metadata handlers, if someone enters wrong handler it still register that metadata handler.
There was a problem hiding this comment.
Ah, I see. I still think a bool is nicer, since you don't have to repeat yourself for each option.
There was a problem hiding this comment.
individual feature names as well as model names should both be available during init.
I couldn't find a list of available feature names/models. Can you provide a link for me please?
Thanks
| metadata_cfg->data = s->cb; | ||
| metadata_cfg->callback = &set_meta; | ||
|
|
||
| err = vmaf_register_metadata_handler(s->vmaf, *metadata_cfg); |
There was a problem hiding this comment.
Anywhere we're using these new libvmaf metadata apis, we'll want to detect if they are available and guard with macros. See CONFIG_LIBVMAF_CUDA_FILTER for an example. This macro is generated during the configure stage.
There was a problem hiding this comment.
The reason for this is compatibility with older libvmaf versions. fyi, these new apis are not part of a libvmaf release yet.
There was a problem hiding this comment.
Anywhere we're using these new libvmaf metadata apis, we'll want to detect if they are available and guard with macros. See CONFIG_LIBVMAF_CUDA_FILTER for an example. This macro is generated during the configure stage.
Should those changes need to own functions like libvmaf_cuda or putting guard macros like I did is fine?
| unsigned frame_number; | ||
| unsigned propagated_handlers_cnt; | ||
| struct FrameList *next; | ||
| } FrameList; |
There was a problem hiding this comment.
I wonder if we could use something like av_fifo here instead?
There was a problem hiding this comment.
Oh yes! Thats what I was looking for!
There was a problem hiding this comment.
@kylophone I make some experiments with av_fifo and that might be not a good idea. Vmaf can send frames any order, If we use FIFO then we should make some operations to send them in order to next filter.
7702810 to
35f34a0
Compare
| {"n_subsample", "Set interval for frame subsampling used when computing vmaf.", OFFSET(n_subsample), AV_OPT_TYPE_INT, {.i64=1}, 1, UINT_MAX, FLAGS}, | ||
| {"model", "Set the model to be used for computing vmaf.", OFFSET(model_cfg), AV_OPT_TYPE_STRING, {.str="version=vmaf_v0.6.1"}, 0, 1, FLAGS}, | ||
| {"feature", "Set the feature to be used for computing vmaf.", OFFSET(feature_cfg), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, FLAGS}, | ||
| {"metadata_handler", "Set the feature to be propagated as metadata.", OFFSET(metadata_feature_cfg), AV_OPT_TYPE_STRING, {.str="name=vmaf"}, 0, 1, FLAGS}, |
There was a problem hiding this comment.
Ah, I see. I still think a bool is nicer, since you don't have to repeat yourself for each option.
| int n_subsample; | ||
| char *model_cfg; | ||
| #if CONFIG_LIBVMAF_METADATA_FILTER | ||
| char *feature_cfg; |
There was a problem hiding this comment.
feature_cfg should not be inside here
| {"n_subsample", "Set interval for frame subsampling used when computing vmaf.", OFFSET(n_subsample), AV_OPT_TYPE_INT, {.i64=1}, 1, UINT_MAX, FLAGS}, | ||
| {"model", "Set the model to be used for computing vmaf.", OFFSET(model_cfg), AV_OPT_TYPE_STRING, {.str="version=vmaf_v0.6.1"}, 0, 1, FLAGS}, | ||
| {"feature", "Set the feature to be used for computing vmaf.", OFFSET(feature_cfg), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, FLAGS}, | ||
| {"metadata_handler", "Set the feature to be propagated as metadata.", OFFSET(metadata_feature_cfg), AV_OPT_TYPE_STRING, {.str="name=vmaf"}, 0, 1, FLAGS}, |
libavfilter/vf_libvmaf.c
Outdated
| }; | ||
| #endif | ||
|
|
||
| #if CONFIG_LIBVMAF_METADATA_FILTER |
There was a problem hiding this comment.
This is unneeded. Not sure if this was intentional, or just a copy/paste error.
There was a problem hiding this comment.
Yes this is copy/paste error. Fixed.
8803754 to
a657cf3
Compare
libavfilter/vf_libvmaf.c
Outdated
| #if CONFIG_LIBVMAF_METADATA_FILTER | ||
| const AVFilter ff_vf_libvmaf_metadata = { | ||
| .name = "libvmaf_cuda", | ||
| .name = "libvmaf_metadata", |
There was a problem hiding this comment.
We don't need this extra filter at all. The only thing we need is to guard the option in the normal "libvmaf" filter.
There was a problem hiding this comment.
I couldn't understand, could you explain more? You want me do something like this; libvmaf_filter_deps="libvmaf libvmaf-metadata"
instead of making
libvmaf_filter_deps="libvmaf"
libvmaf_metadata_filter_deps="libvmaf libvmaf_metadata" in the configure ? If answer is yes, in this way libvmaf filter doesn't work if not it's latest commit.
I couldn't find another way to do this in configuration stage without defining a new filter. If I not define a filter that macro (CONFIG_LIBVMAF_METADATA_FILTER) doesn't generate. I also tried with VMAF_VERSION but since it's string I can't do in macro stage.
This version directly propagates to
AVFrame->metadata. However there are some offset in the frame indexes and doesn't finish at frame index supposed to finish.For example, Lets say I have an input that 30 fps. If I just pass 2 seconds of that file with
-t 2command, filter should take 60 frames, but in current situation it takes60 + first vmaf calculated indexframes (in my case that was 13. In other words firstvmafcalculated and triggered the callback function at 13th frame, 60 + 13 = 73). This problem looks like related toff_filter_framefunction.I think it should only be called inside the
do_vmaffunction, but I couldn't find a way that works like that.Update
I think that sync loss happens because of the framesync. My guess is framesync generating(requesting) frames again when it loss the sync. I am still investigating.
Update 2
Those extra frames actually looks like not re-requested frames. They are actual frames of the input. Program doesn't obeying the command that given (
-t 2) and giving extra frames that shouldn't give.Tested with:
./ffmpeg_g -loglevel info -i images/reference.mp4 -i images/distorted.mp4 -t 2 -lavfi psnr='stats_file=-:stats_version=2' -f null -"And
./ffmpeg_g -loglevel info -i images/reference.mp4 -i images/distorted.mp4 -t 1 -lavfi libvmaf='n_threads=30:feature=name=psnr|name=ciede:metadata_handler=name=vmaf|name=psnr_y|name=ciede2000' -f null -Update 3
It works perfectly fine If I don't give
-tcommand. Instead of I trimmed the files and I gave like that. Right now it works like supposed to be.It also works with complex filtergraphs
./ffmpeg_g -loglevel debug -i images/refer ence.mp4 -i images/distorted.mp4 -filter_complex "[0:v]trim=duration=1, setpts=PTS-STARTPTS[ref];[1:v]trim=duration=1, setpts=PTS-STARTPTS[dist];[ref][dist]libvmaf" -f null -