Netflix / vmaf

Perceptual video quality assessment based on multi-method fusion.
Other
4.61k stars 752 forks source link

Memory leak for n_subsample > 1 #563

Closed titardrew closed 4 years ago

titardrew commented 4 years ago

Hi, I think I have found a memory leak in the new API (libvmaf.rc.c) for the case when n_subsample is greater than 1. The following method does not unref its VmafPicture's.

static int threaded_read_pictures(VmafContext *vmaf, VmafPicture *ref,
                                  VmafPicture *dist, unsigned index)

It skips the loop iteration when n_subsample>1 and iteration % n_subsample != 0 right after the vmaf_picture_ref calls so the pictures they never get unrefed. It seems that they are freed in thread_extract_func after becoming useless for the thread pool. Thus, to solve the issue one should just move vmaf_picture_ref calls below n_subsample if: From:

    for (unsigned i = 0; i < vmaf->registered_feature_extractors.cnt; i++) {
        VmafPicture pic_a, pic_b;
        vmaf_picture_ref(&pic_a, ref);
        vmaf_picture_ref(&pic_b, dist);

        VmafFeatureExtractor *fex =
            vmaf->registered_feature_extractors.fex_ctx[i]->fex;

        if ((vmaf->cfg.n_subsample > 1) && (index % vmaf->cfg.n_subsample) &&
            !(fex->flags & VMAF_FEATURE_EXTRACTOR_TEMPORAL))
        {
            continue;
        }

        VmafFeatureExtractorContext *fex_ctx;
        err = vmaf_fex_ctx_pool_aquire(vmaf->fex_ctx_pool, fex, &fex_ctx);
        if (err) return err;

To:

    for (unsigned i = 0; i < vmaf->registered_feature_extractors.cnt; i++) {
        VmafPicture pic_a, pic_b;

        VmafFeatureExtractor *fex =
            vmaf->registered_feature_extractors.fex_ctx[i]->fex;

        if ((vmaf->cfg.n_subsample > 1) && (index % vmaf->cfg.n_subsample) &&
            !(fex->flags & VMAF_FEATURE_EXTRACTOR_TEMPORAL))
        {
            continue;
        }

        vmaf_picture_ref(&pic_a, ref);
        vmaf_picture_ref(&pic_b, dist);

        VmafFeatureExtractorContext *fex_ctx;
        err = vmaf_fex_ctx_pool_aquire(vmaf->fex_ctx_pool, fex, &fex_ctx);
        if (err) return err;

I hope it would help. Also, it would be great if there were tests covering custom n_subsamples to avoid such regressions, as the feature is quite useful to me.

kylophone commented 4 years ago

Thanks for the bug report. Fixed now.