* [FFmpeg-devel] [PATCH] avformat/mov: parse the last moof box when mp4 segment format @ 2022-09-01 10:22 1035567130 2022-09-27 12:23 ` wangyaqiang 2022-09-29 8:29 ` "zhilizhao(赵志立)" 0 siblings, 2 replies; 6+ messages in thread From: 1035567130 @ 2022-09-01 10:22 UTC (permalink / raw) To: ffmpeg-devel; +Cc: Wang Yaqiang 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". ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat/mov: parse the last moof box when mp4 segment format 2022-09-01 10:22 [FFmpeg-devel] [PATCH] avformat/mov: parse the last moof box when mp4 segment format 1035567130 @ 2022-09-27 12:23 ` wangyaqiang 2022-09-29 8:29 ` "zhilizhao(赵志立)" 1 sibling, 0 replies; 6+ messages in thread From: wangyaqiang @ 2022-09-27 12:23 UTC (permalink / raw) To: FFmpeg development discussions and patches 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". ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat/mov: parse the last moof box when mp4 segment format 2022-09-01 10:22 [FFmpeg-devel] [PATCH] avformat/mov: parse the last moof box when mp4 segment format 1035567130 2022-09-27 12:23 ` wangyaqiang @ 2022-09-29 8:29 ` "zhilizhao(赵志立)" 2022-09-30 9:50 ` wangyaqiang 1 sibling, 1 reply; 6+ messages in thread From: "zhilizhao(赵志立)" @ 2022-09-29 8:29 UTC (permalink / raw) To: FFmpeg development discussions and patches; +Cc: Derek Buitenhuis, Wang Yaqiang > 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". ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat/mov: parse the last moof box when mp4 segment format 2022-09-29 8:29 ` "zhilizhao(赵志立)" @ 2022-09-30 9:50 ` wangyaqiang 2022-09-30 10:22 ` "zhilizhao(赵志立)" 0 siblings, 1 reply; 6+ messages in thread From: wangyaqiang @ 2022-09-30 9:50 UTC (permalink / raw) To: FFmpeg development discussions and patches; +Cc: Derek Buitenhuis > 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". ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat/mov: parse the last moof box when mp4 segment format 2022-09-30 9:50 ` wangyaqiang @ 2022-09-30 10:22 ` "zhilizhao(赵志立)" 2022-09-30 11:47 ` "zhilizhao(赵志立)" 0 siblings, 1 reply; 6+ messages in thread From: "zhilizhao(赵志立)" @ 2022-09-30 10:22 UTC (permalink / raw) To: FFmpeg development discussions and patches > 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". ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat/mov: parse the last moof box when mp4 segment format 2022-09-30 10:22 ` "zhilizhao(赵志立)" @ 2022-09-30 11:47 ` "zhilizhao(赵志立)" 0 siblings, 0 replies; 6+ messages in thread From: "zhilizhao(赵志立)" @ 2022-09-30 11:47 UTC (permalink / raw) To: FFmpeg development discussions and patches > 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". ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-30 11:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-01 10:22 [FFmpeg-devel] [PATCH] avformat/mov: parse the last moof box when mp4 segment format 1035567130 2022-09-27 12:23 ` wangyaqiang 2022-09-29 8:29 ` "zhilizhao(赵志立)" 2022-09-30 9:50 ` wangyaqiang 2022-09-30 10:22 ` "zhilizhao(赵志立)" 2022-09-30 11:47 ` "zhilizhao(赵志立)"
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel This inbox may be cloned and mirrored by anyone: git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \ ffmpegdev@gitmailbox.com public-inbox-index ffmpegdev Example config snippet for mirrors. AGPL code for this site: git clone https://public-inbox.org/public-inbox.git