[FFmpeg-devel] [PATCH] avformat/mov: discard data streams with all zero sample_delta

[Gyan Doshi]

On 2022-07-06 10:54 am, "zhilizhao(赵志立)" wrote:
>
>> On Jul 6, 2022, at 12:09 PM, Gyan Doshi <ffmpeg at gyani.pro> wrote:
>>
>>
>>
>> On 2022-07-06 08:23 am, "zhilizhao(赵志立)" wrote:
>>>> On Jul 5, 2022, at 10:33 PM, Gyan Doshi <ffmpeg at gyani.pro> wrote:
>>>>
>>>>
>>>>
>>>> On 2022-07-05 07:05 pm, "zhilizhao(赵志立)" wrote:
>>>>>> On Jul 5, 2022, at 8:07 PM, Gyan Doshi <ffmpeg at gyani.pro> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2022-07-05 01:20 pm, Zhao Zhili wrote:
>>>>>>> From: Zhao Zhili <zhilizhao at tencent.com>
>>>>>>>
>>>>>>> Streams with all zero sample_delta in 'stts' have all zero dts.
>>>>>>> They have higher chance be chose by mov_find_next_sample(), which
>>>>>>> leads to seek again and again.
>>>>>>>
>>>>>>> For example, GoPro created a 'GoPro SOS' stream:
>>>>>>>    Stream #0:4[0x5](eng): Data: none (fdsc / 0x63736466), 13 kb/s (default)
>>>>>>>      Metadata:
>>>>>>>        creation_time   : 2022-06-21T08:49:19.000000Z
>>>>>>>        handler_name    : GoPro SOS
>>>>>>>
>>>>>>> With 'ffprobe -show_frames http://example.com/gopro.mp4', ffprobe
>>>>>>> blocks until all samples in 'GoPro SOS' stream are consumed first.
>>>>>>>
>>>>>>> Signed-off-by: Zhao Zhili <zhilizhao at tencent.com>
>>>>>>> ---
>>>>>>>   libavformat/mov.c | 14 ++++++++++++++
>>>>>>>   1 file changed, 14 insertions(+)
>>>>>>>
>>>>>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>>>>>> index 88669faa70..2a4eb79f27 100644
>>>>>>> --- a/libavformat/mov.c
>>>>>>> +++ b/libavformat/mov.c
>>>>>>> @@ -3062,6 +3062,20 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>>>>>       st->nb_frames= total_sample_count;
>>>>>>>       if (duration)
>>>>>>>           st->duration= FFMIN(st->duration, duration);
>>>>>>> +
>>>>>>> +    // All samples have zero duration. They have higher chance be chose by
>>>>>>> +    // mov_find_next_sample, which leads to seek again and again.
>>>>>>> +    //
>>>>>>> +    // It's AVERROR_INVALIDDATA actually, but such files exist in the wild.
>>>>>>> +    // So only mark data stream as discarded for safety.
>>>>>>> +    if (!duration && sc->stts_count &&
>>>>>>> +            st->codecpar->codec_type == AVMEDIA_TYPE_DATA) {
>>>>>>> +        av_log(c->fc, AV_LOG_WARNING,
>>>>>>> +               "All samples in data stream index:id [%d:%d] have zero duration, "
>>>>>>> +               "discard the stream\n",
>>>>>>> +               st->index, st->id);
>>>>>>> +        st->discard = AVDISCARD_ALL;
>>>>>>> +    }
>>>>>>>       sc->track_end = duration;
>>>>>>>       return 0;
>>>>>>>   }
>>>>>> So this will allow audio and video streams to be demuxed, but not data?  That distinction seems arbitrary.
>>>>> Disable audio/video streams may create regression. It’s unlikely for random
>>>>> and broken data stream.
>>>>>
>>>>>> Print a warning and assign a duration to each sample. Either 1 or if not zero/Inf, st->duration/st->nb_frames.
>>>>> Set sample_duration to 1 doesn’t work. Dts still far behind other streams.
>>>>>
>>>>> Set sample_duration st->duration/st->nb_frames works for me, but I prefer
>>>>> current strategy for the following reasons:
>>>>>
>>>>> 1. AVDISCARD_ALL is more close to AVERROR_INVALIDDATA by giving up instead
>>>>> of trying correction and hope it works, which may not, e.g., st->duration
>>>>> is broken, or bad interleave even though we fixed sample_duration.
>>>> It's not about hoping that it works.  It's about not preventing the user from acquiring the stream payload.
>>>>
>>>> Can you test if setting -discard:d none -i INPUT allows reading the stream with your patch?
>>> Yes it does allow reading the stream. ’stts’ box is parsed during
>>> avformat_find_stream_info(), AVStream->discard flag can be modified
>>> after that. The patch has no effect if user changed AVStream->discard
>>> flag.
>> What's the duration of the demuxed stream?
> The demuxed data track has correct duration since there is a check
> ```
>      if (duration)
>          st->duration= FFMIN(st->duration, duration);
> ```
> st->duration comes from ‘mdhd’ and not overwrite in this case.
>
> Every packet has zero as timestamp, as expected:
>
> ./ffmpeg -debug_ts  -discard:d none -i ~/tmp/gopro.mp4 -map 0:4 -c copy -copy_unknown -f data /tmp/test
>
> demuxer -> ist_index:4 type:data next_dts:0 next_dts_time:0 next_pts:0 next_pts_time:0 pkt_pts:0 pkt_pts_time:0 pkt_dts:0 pkt_dts_time:0 duration:0 duration_time:0 off:0 off_time:0
> demuxer+ffmpeg -> ist_index:4 type:data pkt_pts:0 pkt_pts_time:0 pkt_dts:0 pkt_dts_time:0 duration:0 duration_time:0 off:0 off_time:0
> muxer <- type:data pkt_pts:0 pkt_pts_time:0 pkt_dts:0 pkt_dts_time:0 duration:0 duration_time:0 size:16

Ok, change the log from

"discard the stream"

to

"stream set to be discarded. Override using -discard or AVStream->discard"

Regards,
Gyan