Re: [FFmpeg-devel] [PATCH] avformat/mov: Fix decoding fragmented MP4 with multiple sample entries and empty stsc

[Dimitry Andric <dimitry@unified-streaming.com>]

On 28 Apr 2025, at 13:00, Dimitry Andric <dimitry@unified-streaming.com> wrote:
> 
> On 19 Apr 2025, at 16:27, Dimitry Andric <dimitry@unified-streaming.com> wrote:
>> 
>> On 10 Apr 2025, at 11:03, Dimitry Andric <dimitry@unified-streaming.com> wrote:
>>> 
>>> On 3 Apr 2025, at 22:02, Dimitry Andric <dimitry@unified-streaming.com> wrote:
>>>> 
>>>> When decoding fragmented MP4 files that have an empty stsc box, and
>>>> instead contain sample description indexes in their tfhd boxes, the mov
>>>> demuxer does not notify the decoder whenever the current sample
>>>> description index changes. If the SPS or PPS changed sufficiently, this
>>>> can lead to unexpected decoding errors.
>>>> 
>>>> To fix this, in mov_finalize_packet(), when stsc_data is not available,
>>>> use get_frag_stream_info_from_pkt() to get at the current fragment
>>>> stream info, and retrieve the current sample description index from
>>>> there. Then use that index in a similar manner as the stsc case.
>>>> 
>>>> Signed-off-by: Dimitry Andric <dimitry@unified-streaming.com>
>>>> ---
>>>> libavformat/mov.c | 50 ++++++++++++++++++++++++++++-------------------
>>>> 1 file changed, 30 insertions(+), 20 deletions(-)
>>>> 
>>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>>> index 452690090c..ead89192f4 100644
>>>> --- a/libavformat/mov.c
>>>> +++ b/libavformat/mov.c
>>>> @@ -10756,25 +10756,29 @@ static int mov_switch_root(AVFormatContext *s, int64_t target, int index)
>>>>  return 1;
>>>> }
>>>> 
>>>> -static int mov_change_extradata(AVStream *st, AVPacket *pkt)
>>>> +static int mov_change_extradata(AVStream *st, AVPacket *pkt, int stsd_id)
>>>> {
>>>>  MOVStreamContext *sc = st->priv_data;
>>>>  uint8_t *side, *extradata;
>>>>  int extradata_size;
>>>> 
>>>> -    /* Save the current index. */
>>>> -    sc->last_stsd_index = sc->stsc_data[sc->stsc_index].id - 1;
>>>> +    if (stsd_id > 0 &&
>>>> +        stsd_id - 1 < sc->stsd_count &&
>>>> +        stsd_id - 1 != sc->last_stsd_index) {
>>>> +        /* Save the current index. */
>>>> +        sc->last_stsd_index = stsd_id - 1;
>>>> 
>>>> -    /* Notify the decoder that extradata changed. */
>>>> -    extradata_size = sc->extradata_size[sc->last_stsd_index];
>>>> -    extradata = sc->extradata[sc->last_stsd_index];
>>>> -    if (st->discard != AVDISCARD_ALL && extradata_size > 0 && extradata) {
>>>> -        side = av_packet_new_side_data(pkt,
>>>> -                                       AV_PKT_DATA_NEW_EXTRADATA,
>>>> -                                       extradata_size);
>>>> -        if (!side)
>>>> -            return AVERROR(ENOMEM);
>>>> -        memcpy(side, extradata, extradata_size);
>>>> +        /* Notify the decoder that extradata changed. */
>>>> +        extradata_size = sc->extradata_size[sc->last_stsd_index];
>>>> +        extradata = sc->extradata[sc->last_stsd_index];
>>>> +        if (st->discard != AVDISCARD_ALL && extradata_size > 0 && extradata) {
>>>> +            side = av_packet_new_side_data(pkt,
>>>> +                                           AV_PKT_DATA_NEW_EXTRADATA,
>>>> +                                           extradata_size);
>>>> +            if (!side)
>>>> +                return AVERROR(ENOMEM);
>>>> +            memcpy(side, extradata, extradata_size);
>>>> +        }
>>>>  }
>>>> 
>>>>  return 0;
>>>> @@ -10893,13 +10897,10 @@ static int mov_finalize_packet(AVFormatContext *s, AVStream *st, AVIndexEntry *s
>>>> 
>>>>  /* Multiple stsd handling. */
>>>>  if (sc->stsc_data) {
>>>> -        if (sc->stsc_data[sc->stsc_index].id > 0 &&
>>>> -            sc->stsc_data[sc->stsc_index].id - 1 < sc->stsd_count &&
>>>> -            sc->stsc_data[sc->stsc_index].id - 1 != sc->last_stsd_index) {
>>>> -            int ret = mov_change_extradata(st, pkt);
>>>> -            if (ret < 0)
>>>> -                return ret;
>>>> -        }
>>>> +        int stsd_id = sc->stsc_data[sc->stsc_index].id;
>>>> +        int ret = mov_change_extradata(st, pkt, stsd_id);
>>>> +        if (ret < 0)
>>>> +            return ret;
>>>> 
>>>>      /* Update the stsc index for the next sample */
>>>>      sc->stsc_sample++;
>>>> @@ -10908,6 +10909,15 @@ static int mov_finalize_packet(AVFormatContext *s, AVStream *st, AVIndexEntry *s
>>>>          sc->stsc_index++;
>>>>          sc->stsc_sample = 0;
>>>>      }
>>>> +    } else {
>>>> +        MOVContext *mov = s->priv_data;
>>>> +        MOVFragmentStreamInfo *frag_stream_info = get_frag_stream_info_from_pkt(&mov->frag_index, pkt, sc->id);
>>>> +        if (frag_stream_info) {
>>>> +            int stsd_id = frag_stream_info->stsd_id;
>>>> +            int ret = mov_change_extradata(st, pkt, stsd_id);
>>>> +            if (ret < 0)
>>>> +                return ret;
>>>> +        }
>>>>  }
>>>> 
>>>>  return 0;
>>>> -- 
>>>> 2.43.0
>>>> 
>>> 
>>> Any comments on this patch?
>> 
>> Ping :)
> 
> Is there any particular group of persons that "own" the mov muxer?

Another ping.

-Dimitry

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".