Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RPU header parsing for Profile 7 #133

Merged
merged 1 commit into from
Apr 1, 2022

Conversation

saindriches
Copy link
Contributor

Add the missing nlq_pred_pivot_value for it.
Fix #131 #132, sorry for forgetting to check tests.

@quietvoid
Copy link
Owner

What would lead to believe this is actually in the bitstream?
Nothing breaks because it's hardcoded to 0, what about other values?

@saindriches
Copy link
Contributor Author

saindriches commented Apr 1, 2022

We don’t know why nlq_pred_pivot_value is existed in Profile 7 bitstream, seems unnecessary. But pretty sure partition is 1 for all current profiles. So exp-golomb code 2046 is actually 3 values, and it matches verifier with fix. If current nlq_pred_pivot_value has some other values, current logic may break, just a coincidence to be decoded as one value without problem.

it's hardcoded to 0

My comment in code may cause confusion, sorry about that, the vec![0; 2] is just for convenience, to create a size-2 vector. It’s then overridden by values from bitstream.

@saindriches
Copy link
Contributor Author

Of course in 8 profile those values are still there.

NLQ things only exist in profiles with EL, and profile 8 bitstream doesn’t have it.

@quietvoid
Copy link
Owner

The changes don't seem to work with values other than zero.

@saindriches
Copy link
Contributor Author

Current value in profile 4/7 is 0 and 1023, it should be able to read and write other values, while the convert_to_mel uses hardcoded vec![0, 1023]. Did I miss something?

@quietvoid
Copy link
Owner

It works with nlq_pred_pivot_value=[0, 1023] but not nlq_pred_pivot_value=[1023, 1023], for example.

@saindriches
Copy link
Contributor Author

fel_to_mel_parsed_mode1.bin.zip
A modified ./assets/tests/fel_to_mel.bin, has vec![1023, 1023], should work.

    "nlq_method_idc": 0,
    "nlq_num_pivots_minus2": 0,
    "nlq_pred_pivot_value": [
      1023,
      1023
    ],
    "num_x_partitions_minus1": 0,
    "num_y_partitions_minus1": 0

@quietvoid
Copy link
Owner

Well yes, it works in dovi_tool but Dolby's verifier errors.

@saindriches
Copy link
Contributor Author

Will try to find what's happening later.

@quietvoid
Copy link
Owner

quietvoid commented Apr 1, 2022

The first pivot value seems to always need to be zero.
It would need validation added.

Maybe we should validate that the array matches [0, x <= 2^bitdepth - 1]

Add the missing nlq_pred_pivot_value for it.
@saindriches
Copy link
Contributor Author

We forgot an important thing: it is used to derive nlq_pivot_index or something, so accumulated value should not exceed 2^bitdepth - 1. For example vec![514, 509] will pass the verifier.

@quietvoid
Copy link
Owner

Ah yea, that makes a lot more sense. Thanks.

@quietvoid
Copy link
Owner

Seems fine to merge, I'll fix the rest myself.

@quietvoid quietvoid merged commit ac41ab8 into quietvoid:main Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants