[FFmpeg-devel] [PATCH 2/3] libavformat/hls: add support for SAMPLE-AES decryption in HLS demuxer

[James Almer]

On 1/18/2021 2:39 PM, Nachiket Tarate wrote:
> +typedef struct AVParserContext {
> +    const uint8_t   *buf_in;
> +    const uint8_t   *buf_end;
> +    uint8_t         *buf_out;
> +    int             next_start_code_length;
> +} AVParserContext;

The AV prefix in structs names should not be used for an internal struct 
used in a single file.

[...]

> +/*
> + * Parse 'dec3' EC3SpecificBox
> + */
> +static int parse_dec3(AC3HeaderInfo **phdr, const uint8_t *buf, size_t size)
> +{
> +    GetBitContext gb;
> +    AC3HeaderInfo *hdr;
> +    int err;
> +
> +    int data_rate, fscod, acmod, lfeon;
> +
> +    if (!*phdr)
> +        *phdr = av_mallocz(sizeof(AC3HeaderInfo));

sizeof(AC3HeaderInfo) is not meant to be used outside of libavcodec. 
It's why avpriv_ac3_parse_header() resides in libavcodec and allocates 
it when used from libavformat.

Since parsing the dec3 atom is trivial, you can do it directly in 
ff_hls_parse_audio_setup_info and store the values in local independent 
variables (or even straight to st->codecpar) without the need for an 
AC3HeaderInfo struct.

> +    if (!*phdr)
> +        return AVERROR(ENOMEM);
> +    hdr = *phdr;
> +
> +    err = init_get_bits8(&gb, buf, size);
> +    if (err < 0)
> +        return AVERROR_INVALIDDATA;
> +
> +    data_rate = get_bits(&gb, 13);
> +    skip_bits(&gb, 3);
> +    fscod = get_bits(&gb, 2);
> +    skip_bits(&gb, 10);
> +    acmod = get_bits(&gb, 3);
> +    lfeon = get_bits(&gb, 1);
> +
> +    hdr->sample_rate = eac3_sample_rate_tab[fscod];
> +
> +    hdr->channel_layout = avpriv_ac3_channel_layout_tab[acmod];
> +    if (lfeon)
> +        hdr->channel_layout |= AV_CH_LOW_FREQUENCY;
> +
> +    hdr->channels = av_get_channel_layout_nb_channels(hdr->channel_layout);
> +
> +    hdr->bit_rate = data_rate*1000;
> +
> +    return 0;
> +}
> +
> +int ff_hls_parse_audio_setup_info(AVStream *st, HLSAudioSetupInfo *info)
> +{
> +    int ret = 0;
> +
> +    AC3HeaderInfo *ac3hdr = NULL;
> +
> +    st->codecpar->codec_tag = info->codec_tag;
> +
> +    if (st->codecpar->codec_id == AV_CODEC_ID_AAC)
> +        return 0;
> +
> +    st->codecpar->extradata = av_mallocz(info->setup_data_length + AV_INPUT_BUFFER_PADDING_SIZE);
> +
> +    if (!st->codecpar->extradata)
> +        return AVERROR(ENOMEM);
> +
> +    st->codecpar->extradata_size = info->setup_data_length;
> +
> +    if (st->codecpar->codec_id == AV_CODEC_ID_AC3)
> +        ret = avpriv_ac3_parse_header(&ac3hdr, info->setup_data, info->setup_data_length);

Are you sure this is correct? avpriv_ac3_parse_header() parses raw ac3 
and eac3 frame headers, and if eac3 here is supposedly stored as a dec3 
atom, wouldn't ac3 be the same using a dac3 atom?

> +    else if (st->codecpar->codec_id == AV_CODEC_ID_EAC3)
> +        ret = parse_dec3(&ac3hdr, info->setup_data, info->setup_data_length);
> +    else
> +        return -1;

AVERROR_INVALIDDATA. Is this function going to be called with other 
codec ids to trigger this path?

[...]

> @@ -2019,9 +2075,14 @@ static int hls_read_header(AVFormatContext *s)
>           * on us if they want to.
>           */
>          if (pls->is_id3_timestamped || (pls->n_renditions > 0 && pls->renditions[0]->type == AVMEDIA_TYPE_AUDIO)) {
> +            if (seg && seg->key_type == KEY_SAMPLE_AES && pls->audio_setup_info.setup_data_length > 0 &&
> +                pls->ctx->nb_streams == 1) {
> +                ff_hls_parse_audio_setup_info(pls->ctx->streams[0], &pls->audio_setup_info);

The return value is being ignored.

> +            } else {
>              ret = avformat_find_stream_info(pls->ctx, NULL);
>              if (ret < 0)
>                  goto fail;
> +            }
>          }
>  
>          pls->has_noheader_flag = !!(pls->ctx->ctx_flags & AVFMTCTX_NOHEADER);