diff mbox series

[FFmpeg-devel,v3,2/2] avformat/mov: add use_hoov option

Message ID tencent_A68553075C4A8B8A661FD0A926C4A7D9EA06@qq.com
State New
Headers show
Series None | expand

Commit Message

zhilizhao(赵志立) Dec. 25, 2021, 9:40 a.m. UTC
Fix #8883.
---
 doc/demuxers.texi  | 6 ++++++
 libavformat/isom.h | 1 +
 libavformat/mov.c  | 9 +++++++--
 3 files changed, 14 insertions(+), 2 deletions(-)

Comments

Marton Balint Dec. 25, 2021, 10:36 a.m. UTC | #1
On Sat, 25 Dec 2021, Zhao Zhili wrote:

> Fix #8883.
> ---
> doc/demuxers.texi  | 6 ++++++
> libavformat/isom.h | 1 +
> libavformat/mov.c  | 9 +++++++--
> 3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/doc/demuxers.texi b/doc/demuxers.texi
> index cab8a7072c..276b536ac5 100644
> --- a/doc/demuxers.texi
> +++ b/doc/demuxers.texi
> @@ -713,6 +713,12 @@ specify.
>
> @item decryption_key
> 16-byte key, in hex, to decrypt files encrypted using ISO Common Encryption (CENC/AES-128 CTR; ISO/IEC 23001-7).
> +
> +@item use_hoov
> +Whether to use hoov atom as moov atom to support some broken files. Set it to 0 for disable, 1 for enable,
> +-1 for automatic, which means take hoov as moov in the second pass if moov doesn't been found in the first
> +pass.
> +
> @end table
>
> @subsection Audible AAX
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index ef8f19b18c..7c837811a6 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -305,6 +305,7 @@ typedef struct MOVContext {
>     int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
>     int have_read_mfra_size;
>     uint32_t mfra_size;
> +    int use_hoov;
> } MOVContext;
>
> int ff_mp4_read_descr_len(AVIOContext *pb);
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index ea2f010aa0..82237f930b 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -7325,7 +7325,7 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>             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.type == MKTAG('h','o','o','v') && c->use_hoov == 1) &&
>                 a.size >= 8 &&
>                 c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) {
>                 uint32_t type;
> @@ -7951,8 +7951,12 @@ static int mov_read_header(AVFormatContext *s)
>
>     /* check MOV header */
>     do {
> -        if (mov->moov_retry)
> +        if (mov->moov_retry) {
>             avio_seek(pb, 0, SEEK_SET);
> +            /* reset use_hoov from auto mode to force mode in second pass */
> +            if (mov->use_hoov == -1)
> +                mov->use_hoov = 1;
> +        }
>         if ((err = mov_read_default(mov, pb, atom)) < 0) {
>             av_log(s, AV_LOG_ERROR, "error reading header\n");
>             return err;
> @@ -8593,6 +8597,7 @@ static const AVOption mov_options[] = {
>     { "decryption_key", "The media decryption key (hex)", OFFSET(decryption_key), AV_OPT_TYPE_BINARY, .flags = AV_OPT_FLAG_DECODING_PARAM },
>     { "enable_drefs", "Enable external track support.", OFFSET(enable_drefs), AV_OPT_TYPE_BOOL,
>         {.i64 = 0}, 0, 1, FLAGS },
> +    { "use_hoov", "Take hoov as moov atom, 0 for disable, -1 for auto, 1 means force", OFFSET(use_hoov), AV_OPT_TYPE_INT, {.i64 = 1}, -1, 1, FLAGS },

Kind of unusual that the default is 1 and not -1 (automatic). Is it 
intentional? Does it make sense to find hoov atom first and discard moov? 
Can't that cause problems for valid files? If it can, then maybe -1 should 
be the default, no?

Thanks,
Marton
Steven Liu Dec. 25, 2021, 11 a.m. UTC | #2
> 在 2021年12月25日,18:36,Marton Balint <cus@passwd.hu> 写道:
> 
> 
> 
> On Sat, 25 Dec 2021, Zhao Zhili wrote:
> 
>> Fix #8883.
>> ---
>> doc/demuxers.texi  | 6 ++++++
>> libavformat/isom.h | 1 +
>> libavformat/mov.c  | 9 +++++++--
>> 3 files changed, 14 insertions(+), 2 deletions(-)
>> 
>> diff --git a/doc/demuxers.texi b/doc/demuxers.texi
>> index cab8a7072c..276b536ac5 100644
>> --- a/doc/demuxers.texi
>> +++ b/doc/demuxers.texi
>> @@ -713,6 +713,12 @@ specify.
>> 
>> @item decryption_key
>> 16-byte key, in hex, to decrypt files encrypted using ISO Common Encryption (CENC/AES-128 CTR; ISO/IEC 23001-7).
>> +
>> +@item use_hoov
>> +Whether to use hoov atom as moov atom to support some broken files. Set it to 0 for disable, 1 for enable,
>> +-1 for automatic, which means take hoov as moov in the second pass if moov doesn't been found in the first
>> +pass.
>> +
>> @end table
>> 
>> @subsection Audible AAX
>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>> index ef8f19b18c..7c837811a6 100644
>> --- a/libavformat/isom.h
>> +++ b/libavformat/isom.h
>> @@ -305,6 +305,7 @@ typedef struct MOVContext {
>>    int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
>>    int have_read_mfra_size;
>>    uint32_t mfra_size;
>> +    int use_hoov;
>> } MOVContext;
>> 
>> int ff_mp4_read_descr_len(AVIOContext *pb);
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index ea2f010aa0..82237f930b 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -7325,7 +7325,7 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>            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.type == MKTAG('h','o','o','v') && c->use_hoov == 1) &&
>>                a.size >= 8 &&
>>                c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) {
>>                uint32_t type;
>> @@ -7951,8 +7951,12 @@ static int mov_read_header(AVFormatContext *s)
>> 
>>    /* check MOV header */
>>    do {
>> -        if (mov->moov_retry)
>> +        if (mov->moov_retry) {
>>            avio_seek(pb, 0, SEEK_SET);
>> +            /* reset use_hoov from auto mode to force mode in second pass */
>> +            if (mov->use_hoov == -1)
>> +                mov->use_hoov = 1;
>> +        }
>>        if ((err = mov_read_default(mov, pb, atom)) < 0) {
>>            av_log(s, AV_LOG_ERROR, "error reading header\n");
>>            return err;
>> @@ -8593,6 +8597,7 @@ static const AVOption mov_options[] = {
>>    { "decryption_key", "The media decryption key (hex)", OFFSET(decryption_key), AV_OPT_TYPE_BINARY, .flags = AV_OPT_FLAG_DECODING_PARAM },
>>    { "enable_drefs", "Enable external track support.", OFFSET(enable_drefs), AV_OPT_TYPE_BOOL,
>>        {.i64 = 0}, 0, 1, FLAGS },
>> +    { "use_hoov", "Take hoov as moov atom, 0 for disable, -1 for auto, 1 means force", OFFSET(use_hoov), AV_OPT_TYPE_INT, {.i64 = 1}, -1, 1, FLAGS },
> 
> Kind of unusual that the default is 1 and not -1 (automatic). Is it intentional? Does it make sense to find hoov atom first and discard moov? Can't that cause problems for valid files? If it can, then maybe -1 should be the default, no?
I have sent a patch for same method, and i don’t think this is a better way. I prefer the method: 
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20211223071012.52638-1-lq@chinaffmpeg.org/

> 
> Thanks,
> Marton
> _______________________________________________
> 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
zhilizhao(赵志立) Dec. 25, 2021, 11:42 a.m. UTC | #3
> On Dec 25, 2021, at 6:36 PM, Marton Balint <cus@passwd.hu> wrote:
> 
> On Sat, 25 Dec 2021, Zhao Zhili wrote:
> 
>> Fix #8883.
>> ---
>> doc/demuxers.texi  | 6 ++++++
>> libavformat/isom.h | 1 +
>> libavformat/mov.c  | 9 +++++++--
>> 3 files changed, 14 insertions(+), 2 deletions(-)
>> 
>> diff --git a/doc/demuxers.texi b/doc/demuxers.texi
>> index cab8a7072c..276b536ac5 100644
>> --- a/doc/demuxers.texi
>> +++ b/doc/demuxers.texi
>> @@ -713,6 +713,12 @@ specify.
>> 
>> @item decryption_key
>> 16-byte key, in hex, to decrypt files encrypted using ISO Common Encryption (CENC/AES-128 CTR; ISO/IEC 23001-7).
>> +
>> +@item use_hoov
>> +Whether to use hoov atom as moov atom to support some broken files. Set it to 0 for disable, 1 for enable,
>> +-1 for automatic, which means take hoov as moov in the second pass if moov doesn't been found in the first
>> +pass.
>> +
>> @end table
>> 
>> @subsection Audible AAX
>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>> index ef8f19b18c..7c837811a6 100644
>> --- a/libavformat/isom.h
>> +++ b/libavformat/isom.h
>> @@ -305,6 +305,7 @@ typedef struct MOVContext {
>>    int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
>>    int have_read_mfra_size;
>>    uint32_t mfra_size;
>> +    int use_hoov;
>> } MOVContext;
>> 
>> int ff_mp4_read_descr_len(AVIOContext *pb);
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index ea2f010aa0..82237f930b 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -7325,7 +7325,7 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>            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.type == MKTAG('h','o','o','v') && c->use_hoov == 1) &&
>>                a.size >= 8 &&
>>                c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) {
>>                uint32_t type;
>> @@ -7951,8 +7951,12 @@ static int mov_read_header(AVFormatContext *s)
>> 
>>    /* check MOV header */
>>    do {
>> -        if (mov->moov_retry)
>> +        if (mov->moov_retry) {
>>            avio_seek(pb, 0, SEEK_SET);
>> +            /* reset use_hoov from auto mode to force mode in second pass */
>> +            if (mov->use_hoov == -1)
>> +                mov->use_hoov = 1;
>> +        }
>>        if ((err = mov_read_default(mov, pb, atom)) < 0) {
>>            av_log(s, AV_LOG_ERROR, "error reading header\n");
>>            return err;
>> @@ -8593,6 +8597,7 @@ static const AVOption mov_options[] = {
>>    { "decryption_key", "The media decryption key (hex)", OFFSET(decryption_key), AV_OPT_TYPE_BINARY, .flags = AV_OPT_FLAG_DECODING_PARAM },
>>    { "enable_drefs", "Enable external track support.", OFFSET(enable_drefs), AV_OPT_TYPE_BOOL,
>>        {.i64 = 0}, 0, 1, FLAGS },
>> +    { "use_hoov", "Take hoov as moov atom, 0 for disable, -1 for auto, 1 means force", OFFSET(use_hoov), AV_OPT_TYPE_INT, {.i64 = 1}, -1, 1, FLAGS },
> 
> Kind of unusual that the default is 1 and not -1 (automatic). Is it intentional? Does it make sense to find hoov atom first and discard moov? Can't that cause problems for valid files? If it can, then maybe -1 should be the default, no?

I prefer to set it to -1 and handle it automatically, but it may break those files Derek mentioned:

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

So force taking hoov as moov is for backward compatibility. It’s pain to workaround another workaround.

> 
> Thanks,
> Marton
> _______________________________________________
> 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".
Steven Liu Dec. 25, 2021, 11:52 a.m. UTC | #4
> 在 2021年12月25日,19:42,zhilizhao(赵志立) <quinkblack@foxmail.com> 写道:
> 
> 
> 
>> On Dec 25, 2021, at 6:36 PM, Marton Balint <cus@passwd.hu> wrote:
>> 
>> On Sat, 25 Dec 2021, Zhao Zhili wrote:
>> 
>>> Fix #8883.
>>> ---
>>> doc/demuxers.texi  | 6 ++++++
>>> libavformat/isom.h | 1 +
>>> libavformat/mov.c  | 9 +++++++--
>>> 3 files changed, 14 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/doc/demuxers.texi b/doc/demuxers.texi
>>> index cab8a7072c..276b536ac5 100644
>>> --- a/doc/demuxers.texi
>>> +++ b/doc/demuxers.texi
>>> @@ -713,6 +713,12 @@ specify.
>>> 
>>> @item decryption_key
>>> 16-byte key, in hex, to decrypt files encrypted using ISO Common Encryption (CENC/AES-128 CTR; ISO/IEC 23001-7).
>>> +
>>> +@item use_hoov
>>> +Whether to use hoov atom as moov atom to support some broken files. Set it to 0 for disable, 1 for enable,
>>> +-1 for automatic, which means take hoov as moov in the second pass if moov doesn't been found in the first
>>> +pass.
>>> +
>>> @end table
>>> 
>>> @subsection Audible AAX
>>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>>> index ef8f19b18c..7c837811a6 100644
>>> --- a/libavformat/isom.h
>>> +++ b/libavformat/isom.h
>>> @@ -305,6 +305,7 @@ typedef struct MOVContext {
>>>   int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
>>>   int have_read_mfra_size;
>>>   uint32_t mfra_size;
>>> +    int use_hoov;
>>> } MOVContext;
>>> 
>>> int ff_mp4_read_descr_len(AVIOContext *pb);
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index ea2f010aa0..82237f930b 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -7325,7 +7325,7 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>           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.type == MKTAG('h','o','o','v') && c->use_hoov == 1) &&
>>>               a.size >= 8 &&
>>>               c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) {
>>>               uint32_t type;
>>> @@ -7951,8 +7951,12 @@ static int mov_read_header(AVFormatContext *s)
>>> 
>>>   /* check MOV header */
>>>   do {
>>> -        if (mov->moov_retry)
>>> +        if (mov->moov_retry) {
>>>           avio_seek(pb, 0, SEEK_SET);
>>> +            /* reset use_hoov from auto mode to force mode in second pass */
>>> +            if (mov->use_hoov == -1)
>>> +                mov->use_hoov = 1;
>>> +        }
>>>       if ((err = mov_read_default(mov, pb, atom)) < 0) {
>>>           av_log(s, AV_LOG_ERROR, "error reading header\n");
>>>           return err;
>>> @@ -8593,6 +8597,7 @@ static const AVOption mov_options[] = {
>>>   { "decryption_key", "The media decryption key (hex)", OFFSET(decryption_key), AV_OPT_TYPE_BINARY, .flags = AV_OPT_FLAG_DECODING_PARAM },
>>>   { "enable_drefs", "Enable external track support.", OFFSET(enable_drefs), AV_OPT_TYPE_BOOL,
>>>       {.i64 = 0}, 0, 1, FLAGS },
>>> +    { "use_hoov", "Take hoov as moov atom, 0 for disable, -1 for auto, 1 means force", OFFSET(use_hoov), AV_OPT_TYPE_INT, {.i64 = 1}, -1, 1, FLAGS },
>> 
>> Kind of unusual that the default is 1 and not -1 (automatic). Is it intentional? Does it make sense to find hoov atom first and discard moov? Can't that cause problems for valid files? If it can, then maybe -1 should be the default, no?
> 
> I prefer to set it to -1 and handle it automatically, but it may break those files Derek mentioned:
> 
> http://ffmpeg.org/pipermail/ffmpeg-devel/2021-December/290359.html
> 
> So force taking hoov as moov is for backward compatibility. It’s pain to workaround another workaround.
https://ffmpeg.org/pipermail/ffmpeg-devel/2021-December/290245.html

This can fix both problem, and will not modify any original main workflow, and the simplest way,

I think you have change the main workflow to fix one or two bug, too complex.

> 
>> 
>> Thanks,
>> Marton
>> _______________________________________________
>> 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".

Thanks
Steven
diff mbox series

Patch

diff --git a/doc/demuxers.texi b/doc/demuxers.texi
index cab8a7072c..276b536ac5 100644
--- a/doc/demuxers.texi
+++ b/doc/demuxers.texi
@@ -713,6 +713,12 @@  specify.
 
 @item decryption_key
 16-byte key, in hex, to decrypt files encrypted using ISO Common Encryption (CENC/AES-128 CTR; ISO/IEC 23001-7).
+
+@item use_hoov
+Whether to use hoov atom as moov atom to support some broken files. Set it to 0 for disable, 1 for enable,
+-1 for automatic, which means take hoov as moov in the second pass if moov doesn't been found in the first
+pass.
+
 @end table
 
 @subsection Audible AAX
diff --git a/libavformat/isom.h b/libavformat/isom.h
index ef8f19b18c..7c837811a6 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -305,6 +305,7 @@  typedef struct MOVContext {
     int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
     int have_read_mfra_size;
     uint32_t mfra_size;
+    int use_hoov;
 } MOVContext;
 
 int ff_mp4_read_descr_len(AVIOContext *pb);
diff --git a/libavformat/mov.c b/libavformat/mov.c
index ea2f010aa0..82237f930b 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -7325,7 +7325,7 @@  static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
             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.type == MKTAG('h','o','o','v') && c->use_hoov == 1) &&
                 a.size >= 8 &&
                 c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) {
                 uint32_t type;
@@ -7951,8 +7951,12 @@  static int mov_read_header(AVFormatContext *s)
 
     /* check MOV header */
     do {
-        if (mov->moov_retry)
+        if (mov->moov_retry) {
             avio_seek(pb, 0, SEEK_SET);
+            /* reset use_hoov from auto mode to force mode in second pass */
+            if (mov->use_hoov == -1)
+                mov->use_hoov = 1;
+        }
         if ((err = mov_read_default(mov, pb, atom)) < 0) {
             av_log(s, AV_LOG_ERROR, "error reading header\n");
             return err;
@@ -8593,6 +8597,7 @@  static const AVOption mov_options[] = {
     { "decryption_key", "The media decryption key (hex)", OFFSET(decryption_key), AV_OPT_TYPE_BINARY, .flags = AV_OPT_FLAG_DECODING_PARAM },
     { "enable_drefs", "Enable external track support.", OFFSET(enable_drefs), AV_OPT_TYPE_BOOL,
         {.i64 = 0}, 0, 1, FLAGS },
+    { "use_hoov", "Take hoov as moov atom, 0 for disable, -1 for auto, 1 means force", OFFSET(use_hoov), AV_OPT_TYPE_INT, {.i64 = 1}, -1, 1, FLAGS },
 
     { NULL },
 };