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

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

[1035567130]

From: Wang Yaqiang <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>
---
 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;
@@ -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
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

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

[wangyaqiang]

ping

> 2022年9月1日 18:22，1035567130@qq.com 写道：
> 
> From: Wang Yaqiang <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>
> ---
> 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;
> @@ -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
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

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

["zhilizhao(赵志立)"]

> On Sep 1, 2022, at 18:22, 1035567130@qq.com wrote:
> 
> From: Wang Yaqiang <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>
> ---
> 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.

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. 

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

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

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

[wangyaqiang]

> 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？

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

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

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

["zhilizhao(赵志立)"]

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

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

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

["zhilizhao(赵志立)"]

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