quietvoid / dovi_tool

dovi_tool is a CLI tool combining multiple utilities for working with Dolby Vision.
MIT License
623 stars 59 forks source link

Fix RPU header parsing for Profile 7 #133

Closed saindriches closed 2 years ago

saindriches commented 2 years ago

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

quietvoid commented 2 years ago

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

saindriches commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

saindriches commented 2 years ago

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 commented 2 years ago

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

saindriches commented 2 years ago

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 commented 2 years ago

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

saindriches commented 2 years ago

Will try to find what's happening later.

quietvoid commented 2 years ago

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]

saindriches commented 2 years ago

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 commented 2 years ago

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

quietvoid commented 2 years ago

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