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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
