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

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

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