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

[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.


> 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.

> This is less of a hack than reordering the nalus.
> (This of course also needs a check to rule out leaks: After all, the
> preceding packet might have given us an rpu, but no first slice. Btw:
> That's the reason why this buffer is synced between threads, isn't it?)

Yes that is the only reason it is synce - corrupt or crafted files, as per
James' last review which requested that.

- Derek