[FFmpeg-devel,2/3] avformat/mov: skip hoov box if strict >= normal

The samples I have got have hoov and moov both. Unknown boxes
should be skipped according to the spec. So don't treat hoov as
moov in normal mode.

For backward compatible, a log message has been added to notice
the user to relax the striction if moov doesn't exist.

Fix #8883.
---
 libavformat/mov.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

> 2021年12月24日 下午5:58，Zhao Zhili <quinkblack@foxmail.com> 写道：
> 
> The samples I have got have hoov and moov both. Unknown boxes
> should be skipped according to the spec. So don't treat hoov as
> moov in normal mode.
> 
> For backward compatible, a log message has been added to notice
> the user to relax the striction if moov doesn't exist.
> 
> Fix #8883.
> ---
> libavformat/mov.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index ea2f010aa0..63483740a0 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -7324,10 +7324,11 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>         if (atom.size >= 8) {
>             a.size = avio_rb32(pb);
>             a.type = avio_rl32(pb);
> -            if (((a.type == MKTAG('f','r','e','e') && c->moov_retry) ||
> -                  a.type == MKTAG('h','o','o','v')) &&
> -                a.size >= 8 &&
> -                c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) {
> +            if (((a.type == MKTAG('f','r','e','e') && c->moov_retry &&
> +                        c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) ||
> +                 (a.type == MKTAG('h','o','o','v') &&
> +                        c->fc->strict_std_compliance < FF_COMPLIANCE_NORMAL)) &&
> +                a.size >= 8) {

 I prefer add one option ignore_hoov, let user choose ignore it, because it should more clearly than use stirct.
>                 uint32_t type;
>                 avio_skip(pb, 4);
>                 type = avio_rl32(pb);
> @@ -7340,6 +7341,10 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>                     a.type = MKTAG('m','o','o','v');
>                 }
>             }
> +            if (a.type == MKTAG('h','o','o','v') &&
> +                c->fc->strict_std_compliance >= FF_COMPLIANCE_NORMAL)
> +                av_log(c->fc, AV_LOG_INFO,
> +                        "Skip hoov atom, try decrease -strict if moov doesn't exist.\n");
>             if (atom.type != MKTAG('r','o','o','t') &&
>                 atom.type != MKTAG('m','o','o','v')) {
>                 if (a.type == MKTAG('t','r','a','k') ||
> -- 
> 2.31.1
> 
> _______________________________________________
> 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".
> 

Thanks

Steven Liu

> On Dec 24, 2021, at 6:19 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
> 
>> 2021年12月24日 下午5:58，Zhao Zhili <quinkblack@foxmail.com> 写道：
>> 
>> The samples I have got have hoov and moov both. Unknown boxes
>> should be skipped according to the spec. So don't treat hoov as
>> moov in normal mode.
>> 
>> For backward compatible, a log message has been added to notice
>> the user to relax the striction if moov doesn't exist.
>> 
>> Fix #8883.
>> ---
>> libavformat/mov.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index ea2f010aa0..63483740a0 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -7324,10 +7324,11 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>        if (atom.size >= 8) {
>>            a.size = avio_rb32(pb);
>>            a.type = avio_rl32(pb);
>> -            if (((a.type == MKTAG('f','r','e','e') && c->moov_retry) ||
>> -                  a.type == MKTAG('h','o','o','v')) &&
>> -                a.size >= 8 &&
>> -                c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) {
>> +            if (((a.type == MKTAG('f','r','e','e') && c->moov_retry &&
>> +                        c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) ||
>> +                 (a.type == MKTAG('h','o','o','v') &&
>> +                        c->fc->strict_std_compliance < FF_COMPLIANCE_NORMAL)) &&
>> +                a.size >= 8) {
> 
> I prefer add one option ignore_hoov, let user choose ignore it, because it should more clearly than use stirct.

Add an option works for me too, but it should default to ignore hoov, because:
1. Truncated/broken files are minority compared to normal files (I hope);
2. Support normal files has higher priority than workaround broken files.

We can support both by drop fake moov info after found a real moov, but
it doesn’t deserve the complexity.

I’d like to get more opinions.


>>                uint32_t type;
>>                avio_skip(pb, 4);
>>                type = avio_rl32(pb);
>> @@ -7340,6 +7341,10 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>                    a.type = MKTAG('m','o','o','v');
>>                }
>>            }
>> +            if (a.type == MKTAG('h','o','o','v') &&
>> +                c->fc->strict_std_compliance >= FF_COMPLIANCE_NORMAL)
>> +                av_log(c->fc, AV_LOG_INFO,
>> +                        "Skip hoov atom, try decrease -strict if moov doesn't exist.\n");
>>            if (atom.type != MKTAG('r','o','o','t') &&
>>                atom.type != MKTAG('m','o','o','v')) {
>>                if (a.type == MKTAG('t','r','a','k') ||
>> -- 
>> 2.31.1
>> 
>> _______________________________________________
>> 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".
>> 
> 
> Thanks
> 
> Steven Liu
> 
>> 2021年12月24日 下午5:58，Zhao Zhili <quinkblack@foxmail.com> 写道：
>> 
>> The samples I have got have hoov and moov both. Unknown boxes
>> should be skipped according to the spec. So don't treat hoov as
>> moov in normal mode.
>> 
>> For backward compatible, a log message has been added to notice
>> the user to relax the striction if moov doesn't exist.
>> 
>> Fix #8883.
>> ---
>> libavformat/mov.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index ea2f010aa0..63483740a0 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -7324,10 +7324,11 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>        if (atom.size >= 8) {
>>            a.size = avio_rb32(pb);
>>            a.type = avio_rl32(pb);
>> -            if (((a.type == MKTAG('f','r','e','e') && c->moov_retry) ||
>> -                  a.type == MKTAG('h','o','o','v')) &&
>> -                a.size >= 8 &&
>> -                c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) {
>> +            if (((a.type == MKTAG('f','r','e','e') && c->moov_retry &&
>> +                        c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) ||
>> +                 (a.type == MKTAG('h','o','o','v') &&
>> +                        c->fc->strict_std_compliance < FF_COMPLIANCE_NORMAL)) &&
>> +                a.size >= 8) {
> 
> I prefer add one option ignore_hoov, let user choose ignore it, because it should more clearly than use stirct.
>>                uint32_t type;
>>                avio_skip(pb, 4);
>>                type = avio_rl32(pb);
>> @@ -7340,6 +7341,10 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>                    a.type = MKTAG('m','o','o','v');
>>                }
>>            }
>> +            if (a.type == MKTAG('h','o','o','v') &&
>> +                c->fc->strict_std_compliance >= FF_COMPLIANCE_NORMAL)
>> +                av_log(c->fc, AV_LOG_INFO,
>> +                        "Skip hoov atom, try decrease -strict if moov doesn't exist.\n");
>>            if (atom.type != MKTAG('r','o','o','t') &&
>>                atom.type != MKTAG('m','o','o','v')) {
>>                if (a.type == MKTAG('t','r','a','k') ||
>> -- 
>> 2.31.1
>> 
>> _______________________________________________
>> 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".
>> 
> 
> Thanks
> 
> Steven Liu

"zhilizhao(赵志立)" <quinkblack@foxmail.com> 于2021年12月24日周五 18:38写道：
>
>
>
> > On Dec 24, 2021, at 6:19 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
> >
> >> 2021年12月24日 下午5:58，Zhao Zhili <quinkblack@foxmail.com> 写道：
> >>
> >> The samples I have got have hoov and moov both. Unknown boxes
> >> should be skipped according to the spec. So don't treat hoov as
> >> moov in normal mode.
> >>
> >> For backward compatible, a log message has been added to notice
> >> the user to relax the striction if moov doesn't exist.
> >>
> >> Fix #8883.
> >> ---
> >> libavformat/mov.c | 13 +++++++++----
> >> 1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >> index ea2f010aa0..63483740a0 100644
> >> --- a/libavformat/mov.c
> >> +++ b/libavformat/mov.c
> >> @@ -7324,10 +7324,11 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>        if (atom.size >= 8) {
> >>            a.size = avio_rb32(pb);
> >>            a.type = avio_rl32(pb);
> >> -            if (((a.type == MKTAG('f','r','e','e') && c->moov_retry) ||
> >> -                  a.type == MKTAG('h','o','o','v')) &&
> >> -                a.size >= 8 &&
> >> -                c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) {
> >> +            if (((a.type == MKTAG('f','r','e','e') && c->moov_retry &&
> >> +                        c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) ||
> >> +                 (a.type == MKTAG('h','o','o','v') &&
> >> +                        c->fc->strict_std_compliance < FF_COMPLIANCE_NORMAL)) &&
> >> +                a.size >= 8) {
> >
> > I prefer add one option ignore_hoov, let user choose ignore it, because it should more clearly than use stirct.
>
> Add an option works for me too, but it should default to ignore hoov, because:
> 1. Truncated/broken files are minority compared to normal files (I hope);
> 2. Support normal files has higher priority than workaround broken files.
>
I think about ignore hoov should not default, because it use hoov
default before.
We should attention users pay attention about this change maybe do it
like deprecate warning. Just waning need change default it ignore hoov
and use moov.
But I’m not sure ignore hoov is correct, because not sure user have
the sample file which only have one hoov and have no moov atom in the
files.

> We can support both by drop fake moov info after found a real moov, but
> it doesn’t deserve the complexity.
>
> I’d like to get more opinions.
>
>
> >>                uint32_t type;
> >>                avio_skip(pb, 4);
> >>                type = avio_rl32(pb);
> >> @@ -7340,6 +7341,10 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>                    a.type = MKTAG('m','o','o','v');
> >>                }
> >>            }
> >> +            if (a.type == MKTAG('h','o','o','v') &&
> >> +                c->fc->strict_std_compliance >= FF_COMPLIANCE_NORMAL)
> >> +                av_log(c->fc, AV_LOG_INFO,
> >> +                        "Skip hoov atom, try decrease -strict if moov doesn't exist.\n");
> >>            if (atom.type != MKTAG('r','o','o','t') &&
> >>                atom.type != MKTAG('m','o','o','v')) {
> >>                if (a.type == MKTAG('t','r','a','k') ||
> >> --
> >> 2.31.1
> >>
> >> _______________________________________________
> >> 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".
> >>
> >
> > Thanks
> >
> > Steven Liu
> >
> >> 2021年12月24日 下午5:58，Zhao Zhili <quinkblack@foxmail.com> 写道：
> >>
> >> The samples I have got have hoov and moov both. Unknown boxes
> >> should be skipped according to the spec. So don't treat hoov as
> >> moov in normal mode.
> >>
> >> For backward compatible, a log message has been added to notice
> >> the user to relax the striction if moov doesn't exist.
> >>
> >> Fix #8883.
> >> ---
> >> libavformat/mov.c | 13 +++++++++----
> >> 1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >> index ea2f010aa0..63483740a0 100644
> >> --- a/libavformat/mov.c
> >> +++ b/libavformat/mov.c
> >> @@ -7324,10 +7324,11 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>        if (atom.size >= 8) {
> >>            a.size = avio_rb32(pb);
> >>            a.type = avio_rl32(pb);
> >> -            if (((a.type == MKTAG('f','r','e','e') && c->moov_retry) ||
> >> -                  a.type == MKTAG('h','o','o','v')) &&
> >> -                a.size >= 8 &&
> >> -                c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) {
> >> +            if (((a.type == MKTAG('f','r','e','e') && c->moov_retry &&
> >> +                        c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) ||
> >> +                 (a.type == MKTAG('h','o','o','v') &&
> >> +                        c->fc->strict_std_compliance < FF_COMPLIANCE_NORMAL)) &&
> >> +                a.size >= 8) {
> >
> > I prefer add one option ignore_hoov, let user choose ignore it, because it should more clearly than use stirct.
> >>                uint32_t type;
> >>                avio_skip(pb, 4);
> >>                type = avio_rl32(pb);
> >> @@ -7340,6 +7341,10 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>                    a.type = MKTAG('m','o','o','v');
> >>                }
> >>            }
> >> +            if (a.type == MKTAG('h','o','o','v') &&
> >> +                c->fc->strict_std_compliance >= FF_COMPLIANCE_NORMAL)
> >> +                av_log(c->fc, AV_LOG_INFO,
> >> +                        "Skip hoov atom, try decrease -strict if moov doesn't exist.\n");
> >>            if (atom.type != MKTAG('r','o','o','t') &&
> >>                atom.type != MKTAG('m','o','o','v')) {
> >>                if (a.type == MKTAG('t','r','a','k') ||
> >> --
> >> 2.31.1
> >>
> >> _______________________________________________
> >> 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".
> >>
> >
> > Thanks
> >
> > Steven Liu
>
> _______________________________________________
> 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".

Thanks
Steven

On 12/24/2021 10:46 AM, Steven Liu wrote:
> I think about ignore hoov should not default, because it use hoov
> default before.

I agree - we (Vimeo) get a ton of hoov files with no moov or broken moov -
much more than files with hoov and valid moov (I've never seen any). This
would be quite a breaking change.

> We should attention users pay attention about this change maybe do it
> like deprecate warning. Just waning need change default it ignore hoov
> and use moov.

+1

> But I’m not sure ignore hoov is correct, because not sure user have
> the sample file which only have one hoov and have no moov atom in the
> files.

We have a lot of files with only hoov.

- Derek

> On Dec 25, 2021, at 3:08 AM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
> 
> On 12/24/2021 10:46 AM, Steven Liu wrote:
>> I think about ignore hoov should not default, because it use hoov
>> default before.
> 
> I agree - we (Vimeo) get a ton of hoov files with no moov or broken moov -
> much more than files with hoov and valid moov (I've never seen any). This
> would be quite a breaking change.

The second version should works for both case: with only hoov and with both
box types. Please review.

http://ffmpeg.org/pipermail/ffmpeg-devel/2021-December/290378.html

> 
>> We should attention users pay attention about this change maybe do it
>> like deprecate warning. Just waning need change default it ignore hoov
>> and use moov.
> 
> +1
> 
>> But I’m not sure ignore hoov is correct, because not sure user have
>> the sample file which only have one hoov and have no moov atom in the
>> files.
> 
> We have a lot of files with only hoov.
> 
> - Derek
> _______________________________________________
> 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".