[FFmpeg-devel] [PATCH] avformat/mov: get the correct fragment stream info when decrypting the sample

[quinkblack at foxmail.com]

OK, I didn't see this one before working on ticket #9807. Comments inline.

On 2022-07-22, 1035567130 wrote:



>From: Wang Yaqiang <wangyaqiang03 at kuaishou.com>
>
> When using mov_read_packet get a pkt, and the packet is encrypted,
> it will invoke the cenc_filter func for process the decryption.
> This function find the frag_stream_info according to frag_index.current,
> but the value of frag_index.current is equal to the last read index of moof box,
> this will cause pkt and frag_stream_info mismatch
> 
> Signed-off-by: Wang Yaqiang <wangyaqiang03 at kuaishou.com>
> ---
> libavformat/isom.h |  2 ++
> libavformat/mov.c  | 33 ++++++++++++++++++++++++++-------
> 2 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index f05c2d9c28..d90e16457b 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -137,6 +137,8 @@ typedef struct MOVFragmentStreamInfo {
>      int64_t next_trun_dts;
>      int index_entry;
>      MOVEncryptionIndex *encryption_index;
> +    int stsd_id; // current fragment stsd_id
> +    int64_t max_pos; //current fragment max pos
> } MOVFragmentStreamInfo;
> typedef struct MOVFragmentIndexItem {
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index a09a762d91..c118072931 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -1182,6 +1182,20 @@ static int mov_read_moov(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      return 0; /* now go for mdat */
> }
> +static int get_frag_index_with_pkt(MOVFragmentIndex *frag_index, AVPacket *pkt)
> +{
> +    if (!pkt) return -1;
> +    for (int i = 0; i < frag_index->nb_items;i++) {
> +        MOVFragmentStreamInfo *frag_stream_info = frag_index->item[i].stream_info;
> +        if (frag_stream_info) {
> +            if (pkt->pos + pkt->size <= frag_stream_info->max_pos) {
> +                return i;
> +            }

Coding style: no curly braces in this case.

> +        }
> +    }

The search is inefficient. Use 'current' as a bookmark.

> +    return -1;
> +}
> +
> static MOVFragmentStreamInfo * get_frag_stream_info(
>      MOVFragmentIndex *frag_index,
>      int index,
> @@ -4868,9 +4882,10 @@ static int mov_read_tfhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      av_log(c->fc, AV_LOG_TRACE, "frag flags 0x%x\n", frag->flags);
>      frag_stream_info = get_current_frag_stream_info(&c->frag_index);
> -    if (frag_stream_info)
> +    if (frag_stream_info) {
>          frag_stream_info->next_trun_dts = AV_NOPTS_VALUE;
> -
> +        frag_stream_info->stsd_id = frag->stsd_id;
> +    }
>      return 0;
> }
> @@ -5335,9 +5350,10 @@ static int mov_read_sidx(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>          index = update_frag_index(c, offset);
>          frag_stream_info = get_frag_stream_info(&c->frag_index, index, track_id);
> -        if (frag_stream_info)
> +        if (frag_stream_info) {
>              frag_stream_info->sidx_pts = timestamp;
> -
> +            frag_stream_info->max_pos = offset + size;

Now it depends on sidx, which may not exist. So it doesn't work reliable.

> +        }
>          if (av_sat_add64(offset, size) != offset + (uint64_t)size ||
>              av_sat_add64(pts, duration) != pts + (uint64_t)duration
>          )
> @@ -7101,13 +7117,16 @@ static int cenc_filter(MOVContext *mov, AVStream* st, MOVStreamContext *sc, AVPa
>      MOVEncryptionIndex *encryption_index;
>      AVEncryptionInfo *encrypted_sample;
>      int encrypted_index, ret;
> -
> -    frag_stream_info = get_frag_stream_info(&mov->frag_index, mov->frag_index.current, st->id);
> +    int frag_index = get_frag_index_with_pkt(&mov->frag_index,pkt);
> +    if (frag_index < 0) {
> +        frag_index = mov->frag_index.current;
> +    }

Search should be reliable. Use an unreliable search and fallback to another unreliable bookmark
is still unrealiable.

> +    frag_stream_info = get_frag_stream_info(&mov->frag_index,frag_index, st->id);
>      encrypted_index = current_index;
>      encryption_index = NULL;
>      if (frag_stream_info) {
>          // Note this only supports encryption info in the first sample descriptor.
> -        if (mov->fragment.stsd_id == 1) {
> +        if (frag_stream_info->stsd_id == 1) {

Good catch! It should go in a separate patch.

>              if (frag_stream_info->encryption_index) {
>                  if (!current_index && frag_stream_info->index_entry)
>                      sc->cenc.frag_index_entry_base = frag_stream_info->index_entry;
> --
> 2.33.0