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