[FFmpeg-devel] [PATCH 2/3 v2] avcodec/hevcdec: Export Dolby Vision RPUs as side data

[Andreas Rheinhardt]

Derek Buitenhuis:
> On 11/6/2021 11:36 PM, Andreas Rheinhardt wrote:
>>> +    /*
>>> +     * Due to limitions in avcodec's current frame threading code, it cannot
>>> +     * handle adding frame side data in any place except before the slice has
>>> +     * started decoding. Since Dolby Vision RPUs (which appear as NAL type 62)
>>> +     * are appended to the AU, this is a poblematic. As a hack around this, we
>>> +     * move any RPUs to before the first slice NAL.
>>> +     */
>>> +    if (codec_id == AV_CODEC_ID_HEVC && pkt->nb_nals > 1 && pkt->nals[pkt->nb_nals - 1].type == HEVC_NAL_UNSPEC62 &&
>>
>> 1. This code is also used CBS, not only the HEVC decoder. So
>> unconditionally moving the NAL is unacceptable.
> 
> [...]
> 
>> (Does this type of NAL actually abide by the typical 0x03 escaping
>> scheme? If not, CBS has a problem.)
> 
> It does, so this at least is not a problem.
> 

Good to hear.

> 
>> 2. This is unnecessarily complicated:
>>     H2645NAL tmp = pkt->nals[pkt->nb_nals - 1];
>>     memmove(&pkt->nals[first_non_slice + 1],
>>             &pkt->nals[first_non_slice],
>>             (pkt->nb_nals - 1 - first_non_slice) * sizeof(*pkt->nals));
>>     pkt->nals[first_non_slice] = tmp;
>> (The naming of first_non_slice is actually misleading: If there are
>> slices, then it is the index of the first slice before moving. It would
>> be easier to use an approach as follows:
>>     int dst_index;
>>     for (dst_index = 0; dst_index < pkt->nb_nals - 1; dst_index)
>>         if (pkt->nals[dst_index] < HEVC_NAL_VPS)
>>             break;
>> )
> 
> Will fix, thanks.
> 
>> 3. This may leak in case there are multiple HEVC_NAL_UNSPEC62 in an
>> H2645Packet. (Don't know whether this is legal, but it is unchecked.)
> 
> It's not legal - Dolby Vision requires a single RPU (UNPSEC62) placed at the
> end of the AU. However, that doesn't preclude crafted files or corrupt files
> from having it like that.
> 
>> 4. My preferred solution for this is adding the following after the call
>> to ff_h2645_packet_split():
>>
>>     if (pkt->nb_nals > 1 && pkt->nals[pkt->nb_nals - 1].type ==
>> HEVC_NAL_UNSPEC62 && nal->size > 2 && !nal->nuh_layer_id &&
>> !nal->temporal_id) {
>>         // create rpu_buf here
> 
> I'm not sure this is 100% correct. I believe RPUs must either be the
> last NAL in the AU - in which case this is correct, *or* they must be
> the NAL in the AU before the EOB/EOS NAL, which, by HEVC spec, must be
> last.
> 

There is already a loop that checks for EOB/EOS NALs. You could use it
to check for HEVC_NAL_UNSPEC62 and create an rpu_buf from it before
ff_thread_finish_setup().

- Andreas