[VDPAU] [PATCH] Fix error in sizes of H265 column width and row height, arrays

[José Hiram Soltren]

On 03/13/2015 02:26 PM, Aaron Plattner wrote:
> On 03/13/2015 11:39 AM, José Hiram Soltren wrote:
>> An NVIDIA internal hardware document noted:
>>
>>   #define MAX_TILE_COLS 20
>>   #define MAX_TILE_ROWS 22
>>
>> As of this writing the VDPAU API writes:
>>
>>      /** Only need to set 0..num_tile_columns_minus1. The struct
>>          definition reserves up to the maximum of 22. Invalid values are
>>          ignored. */
>>      uint16_t column_width_minus1[22];
>>      /** Only need to set 0..num_tile_rows_minus1. The struct
>>          definition reserves up to the maximum of 20. Invalid values are
>>          ignored.*/
>>      uint16_t row_height_minus1[20];
>>
>> This is not correct. The correct definitions ought to be:
>>
>>      uint16_t column_width_minus1[20];
>>      uint16_t row_height_minus1[22];
> 
> Dang it.  Oh well.
> 
>> The H.265 Specification does not give an explicit range for the sizes
>> of these arrays. It is possible to calculate an upper limit for a particular
>> video frame implicitly using these equations:
>>
>> MinCbLog2SizeY = log2_min_luma_coding_block_size_minus3 + 3 (7-10)
>> CtbLog2SizeY = MinCbLog2SizeY + log2_diff_max_min_luma_coding_block_size (7-11)
>> CtbSizeY = 1 << CtbLog2SizeY (7-13)
>> PicWidthInCtbsY = Ceil( pic_width_in_luma_samples ÷ CtbSizeY ) (7-15)
>> num_tile_columns_minus1 ϵ [0, PicWidthInCtbsY − 1]
>>
>> (num_tile_rows_minus1 is similar)
>>
>> For a video with:
>> log2_min_luma_coding_block_size_minus3 = 0
>> log2_diff_max_min_luma_coding_block_size = 0
>> pic_width_in_luma_samples = 4096
>>
>> num_tile_columns_minus1 < 512
>>
>> This seems patological. Perhaps we could cap column_width_minus1[] and
>> row_height_minus1[] at 32 or 64 elements apiece if other hardware
>> implementations saw a reason to do so.
>>
>> This change as proposed does not alter the size of VdpPictureInfoHEVC, but
>> it *does* change the ABI. We can either add it as a fixup to the just
>> released VDPAU 1.0, or create a follow-on patch structure. Since few have
>> adopted VdpPictureInfoHEVC since Monday my preference is to fix the
>> existing structure.
> 
> That's my inclination as well.  Maybe to be completely safe, it would be
> prudent to bump the VDP_DECODER_PROFILE_HEVC_* numbers as well, so that
> implementations built against libvdpau 1.1 won't report support for the
> profiles as defined in libvdpau 1.0?

I would just as soon mark libvdpau 1.0 as deprecated. We can address bumping
profile numbers in a follow on change (just in case there are any other changes
needed).

It would be good to formally reserve ranges of profile number for future codecs.

--José

>> ---
>>   include/vdpau/vdpau.h |    8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/vdpau/vdpau.h b/include/vdpau/vdpau.h
>> index 20c6d1f..de21c00 100644
>> --- a/include/vdpau/vdpau.h
>> +++ b/include/vdpau/vdpau.h
>> @@ -3245,13 +3245,13 @@ typedef struct {
>>       /** Only valid if tiles_enabled_flag is set. Ignored otherwise. */
>>       uint8_t uniform_spacing_flag;
>>       /** Only need to set 0..num_tile_columns_minus1. The struct
>> -        definition reserves up to the maximum of 22. Invalid values are
>> +        definition reserves up to the maximum of 20. Invalid values are
>>           ignored. */
>> -    uint16_t column_width_minus1[22];
>> +    uint16_t column_width_minus1[20];
>>       /** Only need to set 0..num_tile_rows_minus1. The struct
>> -        definition reserves up to the maximum of 20. Invalid values are
>> +        definition reserves up to the maximum of 22. Invalid values are
>>           ignored.*/
>> -    uint16_t row_height_minus1[20];
>> +    uint16_t row_height_minus1[22];
>>       /** Only needed if tiles_enabled_flag is set. Invalid values are
>>           ignored. */
>>       uint8_t loop_filter_across_tiles_enabled_flag;
>>
> 
>