[Mesa-dev] [PATCH 10/18] radeon/vcn: add encode header implementations

[Zhang, Boyuan]

Zhang, Boyuan wrote:
>> 
>> On 14/11/17 10:53, Andy Furniss wrote:
>>>> Zhang, Boyuan wrote:
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>> From: Zhang, Boyuan
>>>>> Sent: November-13-17 11:41 AM
>>>>> To: Andy Furniss; Koenig, Christian; Mark Thompson; 
>>>>> mesa-dev at lists.freedesktop.org
>>>>> Subject: RE: [Mesa-dev] [PATCH 10/18] radeon/vcn: add encode header 
>>>>> implementations
>>>>>
>>>>> Zhang, Boyuan wrote:
>>>>>
>>>>>>>>>> diff --git a/src/gallium/drivers/radeon/radeon_vcn_enc_1_2.c
>>>>>>>>>> b/src/gallium/drivers/radeon/radeon_vcn_enc_1_2.c
>>>>>>>>>> index 5170c67..c6dc420 100644
>>>>>>>>>> --- a/src/gallium/drivers/radeon/radeon_vcn_enc_1_2.c
>>>>>>>>>> +++ b/src/gallium/drivers/radeon/radeon_vcn_enc_1_2.c
>>>>>>>>>> @@ -362,6 +362,233 @@ static void radeon_enc_quality_params(struct radeon_encoder *enc)
>>>>>>>>>>          RADEON_ENC_END();
>>>>>>>>>>      }
>>>>>>>>>>      +static void radeon_enc_nalu_sps(struct radeon_encoder 
>>>>>>>>>> *enc) {
>>>>>>>>>> +    RADEON_ENC_BEGIN(RENCODE_IB_PARAM_DIRECT_OUTPUT_NALU);
>>>>>>>>>> +    RADEON_ENC_CS(RENCODE_DIRECT_OUTPUT_NALU_TYPE_SPS);
>>>>>>>>>> +    uint32_t *size_in_bytes = &enc->cs->current.buf[enc->cs->current.cdw++];
>>>>>>>>>> +    radeon_enc_reset(enc);
>>>>>>>>>> +    radeon_enc_set_emulation_prevention(enc, false);
>>>>>>>>>> +    radeon_enc_code_fixed_bits(enc, 0x00000001, 32);
>>>>>>>>>> +    radeon_enc_code_fixed_bits(enc, 0x67, 8);
>>>>>>>>>> +    radeon_enc_byte_align(enc);
>>>>>>>>>> +    radeon_enc_set_emulation_prevention(enc, true);
>>>>>>>>>> +    radeon_enc_code_fixed_bits(enc, enc->enc_pic.spec_misc.profile_idc, 8);
>>>>>>>>>> +    radeon_enc_code_fixed_bits(enc, 0x04, 8);
>>>>>>>>> Please always set constraint_set1_flag when profile_idc is 66.  There are enough actually-constrained-baseline-but-not-marked-as-such streams in the world already to catch out decoders without full baseline support (that is, all of them).
>>>>>>>>>
>>>>>>>>> Also, "constraint_set5_flag shall be equal to 0 when profile_idc is not equal to 77, 88, 100, or 118".
>>>>>>>>
>>>>>>> [Boyuan] Thanks for pointing out. I modified the value to be 0x44 in the new patch (set1=1, and set5=1) since we only support constrained baseline for now.
>>>>>>
>>>>>> It's not really with cabac though. I know there was a patch to turn it off - but that would have been wasteful and make linux < windows.
>>>>>>
>>>>>> Why not use 77 if cabac is on + not set constrained bits, windows seems to set main.
>>>>>>
>>>>>> Currently with vce trying to set main manually from ffmeg/gst in order to get something "correct" still sets flags = something that's not seen as main (but works).
>>>>>
>>>>> Yes, but still the problem is cabac is not allowed for baseline profile and we only support baseline for now. I'm not quite sure about windows test you mentioned, I'm just guessing that we might have some main profile features support in some closed test environment on windows side, but definitely not all main profile features, no b frame support.
>>>>> On linux side, we only support baseline profile for this vcn enc.
>>>>> (fixed a typo)
>>>>
>>>>   Yea, it's tricky, FWIW windows does not use b-frames for the test I've done (re-live turned up high = 50mbit 60fps).
>>>
>>> I can get B-frames from AMF with a GCN 2 card on Windows.
>> 
>> Yes, we used to support b-frame for GCN2 (e.g. like Bonaire, Kabini, etc...) cards. But for later Asics, we dropped the b-frame support. Including this raven vcn encode, b-frame is still not supported according to our firmware team.
>> - Boyuan
>> 
>>>
>>>> It is flagged as main and uses cabac.
>>>> I guess main is the "correct" way to describe constrained baseline + cabac even if there are no other main features like b-frames.
>> 
>> Yes, I agree, that's the correct way in that case. However, we can't advertise main profile since we don't have b-frame support. It would cause serious problem if player tries to use b-frame when seeing that main profile is supported.
>
> I guess you mean encoder rather than player - I don't think a player should care if there are no b-frames in main.
>
> FWIW vainfo does currently advertise main/high support, IIRC it was mentioned in the past but lost in a long thread with many other issues in it.

I remember there was a discussion before about this, I will double check it and will probably make a patch to disable them if not yet disabled.

>
>  So as explained before, we only support constrained baseline profile for this raven vcn encode now. And as a result, cabac is forced to be off in this bring-up patch.
>
> Maybe advertise constrained baseline as encode + expose cabac switch that's in the vaapi spec and let the user/app decide to force that.
If cabac is on then flag the output as main.

Thanks for this idea, I could raise this in our meeting to discuss the possibility of doing this. Currently, "advertising constrained baseline only"  + "enabling cabac" at the same time doesn't sound like a "correct"/"perfect" way to me, I need more discussions with my team about this. Therefore, I will push the current patches, e.g. keep cabac disabled for now, to bring up this vcn encode if you are OK with it.
- Boyuan