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

[Andreas Rheinhardt]

Derek Buitenhuis:
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis at gmail.com>
> ---
>  libavcodec/h2645_parse.c | 28 ++++++++++++++++++++++++++++
>  libavcodec/hevcdec.c     | 29 +++++++++++++++++++++++++++++
>  libavcodec/hevcdec.h     |  2 ++
>  libavcodec/version.h     |  2 +-
>  4 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> index 6fbe97ad4a..7adf5b7ef1 100644
> --- a/libavcodec/h2645_parse.c
> +++ b/libavcodec/h2645_parse.c
> @@ -517,6 +517,34 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>          pkt->nb_nals++;
>      }
>  
> +    /*
> +     * 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.)

> +        !pkt->nals[pkt->nb_nals - 1].nuh_layer_id && !pkt->nals[pkt->nb_nals - 1].temporal_id) {
> +
> +        int first_non_slice = 0;
> +        H2645NAL *tmp = av_malloc(pkt->nb_nals * sizeof(H2645NAL));
> +        if (!tmp)
> +            return AVERROR(ENOMEM);
> +
> +        for (int i = pkt->nb_nals - 1; i >= 0; i--) {
> +            if (pkt->nals[i].type < HEVC_NAL_VPS)
> +                first_non_slice = i;
> +            tmp[i] = pkt->nals[i];
> +        }
> +
> +        pkt->nals[first_non_slice] = pkt->nals[pkt->nb_nals - 1];
> +        for (int i = first_non_slice + 1; i < pkt->nb_nals; i++)
> +            pkt->nals[i] = tmp[i - 1];
> +
> +        av_free(tmp);
> +    }

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;
)

> +
>      return 0;
>  }
>  
> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> index 246ffd7d80..dd286deaa7 100644
> --- a/libavcodec/hevcdec.c
> +++ b/libavcodec/hevcdec.c
> @@ -2950,6 +2950,14 @@ static int set_side_data(HEVCContext *s)
>          }
>      }
>  
> +    if (s->rpu_buf) {
> +        AVFrameSideData *rpu = av_frame_new_side_data_from_buf(out, AV_FRAME_DATA_DOVI_RPU_BUFFER, s->rpu_buf);
> +        if (!rpu)
> +            return AVERROR(ENOMEM);
> +
> +        s->rpu_buf = NULL;
> +    }
> +
>      return 0;
>  }
>  
> @@ -3224,6 +3232,20 @@ static int decode_nal_unit(HEVCContext *s, const H2645NAL *nal)
>      case HEVC_NAL_AUD:
>      case HEVC_NAL_FD_NUT:
>          break;
> +    case HEVC_NAL_UNSPEC62:
> +        /*
> +         * Check for RPU delimiter.
> +         *
> +         * Dolby Vision RPUs masquerade as unregistered NALs of type 62.
> +         */
> +        if (nal->size > 2 && !nal->nuh_layer_id && !nal->temporal_id) {
> +            s->rpu_buf = av_buffer_alloc(nal->raw_size - 2);

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

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

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?)

> +            if (!s->rpu_buf)
> +                return AVERROR(ENOMEM);
> +
> +            memcpy(s->rpu_buf->data, nal->raw_data + 2, nal->raw_size - 2);
> +        }
> +        break;
>      default:
>          av_log(s->avctx, AV_LOG_INFO,
>                 "Skipping NAL unit %d\n", s->nal_unit_type);