Expose builtin models

[ThatNerdUKnow]

Related to issue #1128
the function vmaf_model_load allows you to load a builtin vmaf model, given a VmafModelConfiguration and a "version" string which corresponds to the version property on the VmafBuildInModel struct defined in model.c
This struct and array are not exposed to the public api, meaning that users of libvmaf would have to know the magic strings to pass to the version param of vmaf_model_load to get a VmafModel struct
This PR adds a function vmaf_built_in_model_next which iterates through each item in built_in_models, allowing users of libvmaf to get a list of available built in models

[kylophone]

Thanks for your work on this! Would you please add a new test in test/test_model.c ?

[ThatNerdUKnow]

Thanks for your work on this! Would you please add a new test in test/test_model.c ?

Sure can,
Do you have any guidance on unit testing here?
If it gets a null pointer we get a pointer to a vmafDescriptor?
Do I just call that in a loop until i get back a null?
Do some validation that we are indeed getting a pointer to the version string that we're expecting? How would I go about doing this in a way that won't break whenever the model's version string changes?

EDIT
I think I figured it out

[kylophone]

Could you please fix your diff for test_model.c? The only thing that should be changing is the addition of the test, rest of the file should remain untouched.

[ThatNerdUKnow]

Could you please fix your diff for test_model.c? The only thing that should be changing is the addition of the test, rest of the file should remain untouched.

Sorry about that, my autoformatter must have done its thing without me noticing

[ThatNerdUKnow]

Aside from reverting to that commit and re-doing my changes is there a way to do this?

[kylophone]

Aside from reverting to that commit and re-doing my changes is there a way to do this?

I would just revert your changes to test_model.c and then copy/paste the test_model_descriptor_next() bit at the end.

[kylophone]

From an API perspective, I think this is basically ready to go. A few more items on my side.

• Although I suggested it, I am a bit ambivalent about the struct casting, I need to think a little more about that. It works, but may not be the cleanest solution. Suggestions welcome here.
• The test shows the api functionality, but it is not really testing for anything. Maybe we could also try loading the models to prove that it's working.

[ThatNerdUKnow]

I'm not sure if testing the model loading is entirely appropriate here. While it is definitely relevant it would likely more fall under the testing of vmaf_model_load?
I'm asserting that *VmafModelDescriptor->Version == *VmafBuiltInModel->Version My reasoning is that since the version param of vmaf_model_load corresponds to the version field of one of the VmafBuiltInModels in built_in_models[]

I could attempt to load a model, but it seems redundant assuming that vmaf_model_load is already well tested. However I do see the value in asserting my assumption one step further by saying "Yes, the version field matches, but I should still be able to load a model with this"

[kylophone]

I'm asserting that *VmafModelDescriptor->Version == *VmafBuiltInModel->Version My reasoning is that since the version param of vmaf_model_load corresponds to the version field of one of the VmafBuiltInModels in built_in_models[]

Yes, but I don't think that's what is happening here.

VmafBuiltInModel *builtin = next;
mu_assert("Version field should match on both builtin and next", next->version == builtin->version);

[ThatNerdUKnow]

I think I may have explained myself poorly. vmaf_model_load takes a param named version.
There is an array of VmafBuiltInModels each with a version property.
If you were to take any of these "version" property fields and pass it to vmaf_model_load then you shouldn't encounter an error
So if every VmafBuiltInModel->version will give you a model when passed to vmaf_model_load then if VmafModelDescriptor->Version == VmafBuiltInModel->Version then that too should give you a model when passed to vmaf_model_load
However I see now that it would be useful to make that assertion more explicit by attempting to load a model given a VmafModelDescriptor

[ThatNerdUKnow]

does this look right?

/**
 * This test may fail if VmafBuiltInModel's memory layout has changed
 *  (version MUST be the first defined property in the struct)
 */
char *test_model_descriptor_next()
{
    VmafModelDescriptor *next = NULL;
    VmafModelConfig config = {.name = NULL, .flags = VMAF_MODEL_FLAGS_DEFAULT};
    while (vmaf_model_descriptor_next(next))
    {
        next = vmaf_model_descriptor_next(next);
        VmafBuiltInModel *builtin = next;
        mu_assert("Version field should match on both builtin and next", next->version == builtin->version);
        VmafModel *model = NULL;
        int err = vmaf_model_load(&model, &config, next->version);

        mu_assert("Model load should not return an error",err==0);
        vmaf_model_destroy(model);
    }
    return NULL;
}

This test fails with the message >>> MALLOC_PERTURB_=94 /home/brandon/repos/vmaf/libvmaf/build/test/test_model
I can't decipher this error at all, Am I invoking vmaf_model_load incorrectly?

[ThatNerdUKnow]

I modified the unit test to write the current version that it's trying to load to stdout. Here's what I get

==================================== 4/13 ====================================
test:         test_model
start time:   05:49:15
duration:     0.06s
result:       exit status 1
command:      MALLOC_PERTURB_=221 /home/brandon/repos/vmaf/libvmaf/build/test/test_model
----------------------------------- stdout -----------------------------------
vmaf_v0.6.1
vmaf_b_v0.6.3
----------------------------------- stderr -----------------------------------
test_json_model: �[32mpass�[0m
test_built_in_model: �[32mpass�[0m
test_model_load_and_destroy: �[32mpass�[0m
test_model_check_default_behavior_unset_flags: �[32mpass�[0m
test_model_check_default_behavior_set_flags: �[32mpass�[0m
test_model_set_flags: �[32mpass�[0m
test_model_feature: �[32mpass�[0m
test_model_descriptor_next: �[31mfail�[0m�[31m, Model load should not return an error
8 tests run, 1 failed�[0m
==============================================================================

Looks like it's reading the version string just fine, but for some reason attempting to load vmaf_b_v0.6.3 causes the model load to fail

[ThatNerdUKnow]

@kylophone I believe that this test failing is more a problem with vmaf_model_load or the model itself than with model_descriptor_next given the new assertation that we are able to construct a VmafModel with next->version

[kylophone]

Let me find some time to look at it. Are you OK if I make some changes?

[ThatNerdUKnow]

Let me find some time to look at it. Are you OK if I make some changes?

By all means!

[ThatNerdUKnow]

I ran a debugger session when attempting to load "vmaf_b_v0.6.3" and noticed something interesting. I found that the model is getting caught up in parsing. In this function from read_json_model.c specifically:

(I've added comments in places of note)

static int model_parse(json_stream *s, VmafModel *model,
                       enum VmafModelFlags flags)
{
    int err = -EINVAL;

    if (json_next(s) != JSON_OBJECT)
        return -EINVAL;

    while (json_peek(s) != JSON_OBJECT_END && !json_get_error(s)) {
        if (json_next(s) != JSON_STRING)
            return -EINVAL;
        const char *key = json_get_string(s, NULL); // Read the current "key" value of the json stream

        if (!strcmp(key, "model_dict")) { // If current "key" is "model_dict", continue. This never evaluates to true!
            err = parse_model_dict(s, model, flags);
            if (err) return err;
            continue;
        }

        json_skip(s);
    }

    json_skip_until(s, JSON_OBJECT_END);
    return err;
}

If you take a look at vmaf_b_v0.6.3 you'll see that all the top level keys are "0","1","2", etc...
Each of these keys have a sub-key called "model_dict". This looks like some sort of collection of models.
All the other models that seem to load just fine all have "model_dict" in the root of the json
This probably explains why vmaf_model_load is failing for this model specifically.

[ThatNerdUKnow]

I did some more looking and it looks like built_in_models stores both VmafModels and VmafModelCollections, or rather I should say, both vmaf_model_load and vmaf_model_collection_load both use the built_in_models array to construct VmafModels and VmafModelCollections respectively. The API of vmaf_model_load and vmaf_model_collection_load both expect the caller to know beforehand which type a model file is before calling, which isn't the case in the unit test for vmaf_model_descriptor_next

if we were to split those into seperate arrays we could do the following:

// When next is null, return a pointer to the first item in the new array, built_in_model_collections
VmafModelDescriptor *vmaf_model_collection_next(VmafModelDescriptor *next);

// When next is null, return a pointer to the current array, built_in_models.
VmafModelDescriptor *vmaf_model_next(VmafModelDescriptor *next);

// Not exposed in the headers
// Called into by both vmaf_model_next and vmaf_model_collection_next
// Basically the same logic as now, but doesn't make decisions on what to do when next is null
VmafModelDescriptor *vmaf_model_descriptor_next(VmafModelDescriptor *next);

This might require changing the function signature of these functions slightly though, since giving a null pointer to vmaf_model_descriptor_next is undefined, each one of the functions defined above need to be modified to return int to signify errors. (edit: vmaf_model_descriptor_next could just return null when next is null instead) This shouldn't be a big deal since vmaf_model_descriptor_next isn't currently used and the other two functions defined here don't exist yet. @kylophone Your thoughts?