Re: [FFmpeg-devel] [PATCH] avformat/mov: parse the last moof box when mp4 segment format

["\"zhilizhao(赵志立)\"" <quinkblack@foxmail.com>]

> On Sep 30, 2022, at 18:22, zhilizhao(赵志立) <quinkblack@foxmail.com> wrote:
> 
> 
> 
>> On Sep 30, 2022, at 17:50, wangyaqiang <1035567130@qq.com> wrote:
>> 
>> 
>> 
>>> 2022年9月29日 16:29，zhilizhao(赵志立) <quinkblack@foxmail.com <mailto:quinkblack@foxmail.com>> 写道：
>>> 
>>> 
>>> 
>>>> On Sep 1, 2022, at 18:22, 1035567130@qq.com <mailto:1035567130@qq.com> wrote:
>>>> 
>>>> From: Wang Yaqiang <wangyaqiang03@kuaishou.com <mailto:wangyaqiang03@kuaishou.com>>
>>>> 
>>>> In the format of mp4 segment, the bitrate calculation of
>>>> stream depends on the sample_size in moof->traf->trun box.
>>>> In the original logic, when the last sidx box is read,
>>>> it is not parsed backwards, and the total sample_size calculation is smaller.
>>>> As a result, the bitrate displayed by ffprobe is also smaller than the actual.
>>>> Increasing the moof_count variable ensures that the last moof is parsed.
>>>> 
>>>> Test method: You can use -c copy remux a fmp4 file as mp4
>>>> and ffprobe the two files will find that the bitrate is inconsistent
>>>> Befor patch:
>>>> Stream #0:1[0x2](und): Audio: aac (HE-AAC) (mp4a / 0x6134706D), 44100 Hz, stereo, fltp, 59 kb/s
>>>> After patch:
>>>> Stream #0:1[0x2](und): Audio: aac (HE-AAC) (mp4a / 0x6134706D), 44100 Hz, stereo, fltp, 96 kb/s (default)
>>>> 
>>>> Signed-off-by: Wang Yaqiang <wangyaqiang03@kuaishou.com <mailto:wangyaqiang03@kuaishou.com>>
>>>> ---
>>>> libavformat/isom.h | 1 +
>>>> libavformat/mov.c  | 8 +++++++-
>>>> 2 files changed, 8 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>>>> index f05c2d9c28..183a3c486b 100644
>>>> --- a/libavformat/isom.h
>>>> +++ b/libavformat/isom.h
>>>> @@ -296,6 +296,7 @@ typedef struct MOVContext {
>>>>  int has_looked_for_mfra;
>>>>  int use_tfdt;
>>>>  MOVFragmentIndex frag_index;
>>>> +    int moof_count; //ensure last fragment parse moof box
>>>>  int atom_depth;
>>>>  unsigned int aax_mode;  ///< 'aax' file has been detected
>>>>  uint8_t file_key[20];
>>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>>> index 14550e6456..396658e342 100644
>>>> --- a/libavformat/mov.c
>>>> +++ b/libavformat/mov.c
>>>> @@ -7784,12 +7784,15 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>>          int64_t start_pos = avio_tell(pb);
>>>>          int64_t left;
>>>>          int err = parse(c, pb, a);
>>>> +            if (a.type == MKTAG('m','o','o','f')) {
>>>> +                c->moof_count ++;
>>>> +            }
>>>>          if (err < 0) {
>>>>              c->atom_depth --;
>>>>              return err;
>>>>          }
>>>>          if (c->found_moov && c->found_mdat && a.size <= INT64_MAX - start_pos &&
>>>> -                ((!(pb->seekable & AVIO_SEEKABLE_NORMAL) || c->fc->flags & AVFMT_FLAG_IGNIDX || c->frag_index.complete) ||
>>>> +                ((!(pb->seekable & AVIO_SEEKABLE_NORMAL) || c->fc->flags & AVFMT_FLAG_IGNIDX || (c->frag_index.complete && c->moof_count >= c->frag_index.nb_items)) ||
>>>>               start_pos + a.size == avio_size(pb))) {
>>>>              if (!(pb->seekable & AVIO_SEEKABLE_NORMAL) || c->fc->flags & AVFMT_FLAG_IGNIDX || c->frag_index.complete)
>>>>                  c->next_root_atom = start_pos + a.size;
>>> 
>>> No, it breaks the use case with global sidx. We can achieve fast
>>> startup with global sidx.
>> 
>> This patch indeed have an effect on startup, will add some option.
>> 
>>> 
>>> The patch describes an issue with multiple sidx cases, actually
>>> it’s more serious with global sidx. In the first case, the bitrate
>>> is a little inaccurate. Bitrate is near zero for the second case.
>>> 
>>> I have two ideas:
>>> 
>>> 1. Just skip bitrate calculation from sc->data_size if sidx exist,
>>> e.g.,
>>> 
>>> @@ -8500,7 +8500,7 @@ static int mov_read_header(AVFormatContext *s)
>>>       for (i = 0; i < s->nb_streams; i++) {
>>>           AVStream *st = s->streams[i];
>>>           MOVStreamContext *sc = st->priv_data;
>>> -            if (st->duration > 0) {
>>> +            if (st->duration > 0 && !sc->has_sidx) {
>>>               /* Akin to sc->data_size * 8 * sc->time_scale / st->duration but accounting for overflows. */
>>>               st->codecpar->bit_rate = av_rescale(sc->data_size, ((int64_t) sc->time_scale) * 8, st->duration);
>>>               if (st->codecpar->bit_rate == INT64_MIN) {
>>> 
>>> It’s simple, and bitrate information has multiple sources. 
>>> 
>> 
>> In this way, will be no bitrate info in the stream, will affect the users？
> 
> It depends. We have multiple sources to get bitrate.
> 
>> 
>>> 2. Add a option to prevent mark complete when reading sidx
>>> 
>>> @@ -5446,7 +5446,7 @@ static int mov_read_sidx(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>   // See if the remaining bytes are just an mfra which we can ignore.
>>>   is_complete = offset == stream_size;
>>> -    if (!is_complete && (pb->seekable & AVIO_SEEKABLE_NORMAL) && stream_size > 0 ) {
>>> +    if (!is_complete && (pb->seekable & AVIO_SEEKABLE_NORMAL) && stream_size > 0 && !parse_full) {
>>> 
>>> Then
>> 
>> Mfra box is not required in fmp4. for example, adding skip_trailer parameter when generate fm4, bitrate will still be incorrect.
> 
> You missed the point. If mfra doesn’t exist in the file,
> the issue doesn’t exist neither.

Sorry you are right. offset is not current file position but accumulated
value from sidx boxes.

c->frag_index.complete should be marked conditionally, if we can’t find
a better solution.

> 
>> 
>>> 
>>> @@ -8500,7 +8500,7 @@ static int mov_read_header(AVFormatContext *s)
>>>       for (i = 0; i < s->nb_streams; i++) {
>>>           AVStream *st = s->streams[i];
>>>           MOVStreamContext *sc = st->priv_data;
>>> -            if (st->duration > 0) {
>>> +            if (st->duration > 0 && (!sc->has_sidx || parse_full)) {
>>> 
>>> It’s such a corner case and I can’t find a suitable name for this option.
>>> 
>>> Any ideas?
>>> 
>>>> @@ -8361,6 +8364,9 @@ static int mov_read_header(AVFormatContext *s)
>>>>      av_log(s, AV_LOG_ERROR, "moov atom not found\n");
>>>>      return AVERROR_INVALIDDATA;
>>>>  }
>>>> +    if (mov->frag_index.nb_items > mov->moof_count) {
>>>> +        av_log(s, AV_LOG_WARNING, "the number of moof is less then fragment count\n");
>>>> +    }
>>>>  av_log(mov->fc, AV_LOG_TRACE, "on_parse_exit_offset=%"PRId64"\n", avio_tell(pb));
>>>> 
>>>>  if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
>>>> -- 
>>>> 2.33.0
>>>> 
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
>>>> 
>>>> To unsubscribe, visit link above, or email
>>>> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
>>> 
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
>>> 
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
>> _______________________________________________
>> 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".
> 
> _______________________________________________
> 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".

_______________________________________________
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".