diff mbox series

[FFmpeg-devel] avformat/mov: make compatible brands more readable

Message ID tencent_F23E5956819D96E0445AE835958106349F09@qq.com
State New
Headers show
Series [FFmpeg-devel] avformat/mov: make compatible brands more readable
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Zhao Zhili Sept. 9, 2020, 1:07 p.m. UTC
From: Zhao Zhili <quinkblack@foxmail.com>

Use comma as separator between multiple compatible brands.
---
 libavformat/mov.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Comments

Derek Buitenhuis Sept. 9, 2020, 1:51 p.m. UTC | #1
On 09/09/2020 14:07, quinkblack@foxmail.com wrote:
> From: Zhao Zhili <quinkblack@foxmail.com>
> 
> Use comma as separator between multiple compatible brands.
> ---
>  libavformat/mov.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)

This change appears to actually be several dfferent changes in one,
most of which are not listed in the commit message.

- Derek
James Almer Sept. 9, 2020, 2:24 p.m. UTC | #2
On 9/9/2020 10:07 AM, quinkblack@foxmail.com wrote:
> From: Zhao Zhili <quinkblack@foxmail.com>
> 
> Use comma as separator between multiple compatible brands.

Wont this potentially break parsing of the output of ffmpeg/ffprobe, or
even of the compatible_brands key in c->fc->metadata when using the dict
API?

> ---
>  libavformat/mov.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 690beb10ce..8f5341f925 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -1093,7 +1093,7 @@ static int aax_filter(uint8_t *input, int size, MOVContext *c)
>  static int mov_read_ftyp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  {
>      uint32_t minor_ver;
> -    int comp_brand_size;
> +    int comp_brand_count;
>      char* comp_brands_str;
>      uint8_t type[5] = {0};
>      int ret = ffio_read_size(pb, type, 4);
> @@ -1107,19 +1107,25 @@ static int mov_read_ftyp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      minor_ver = avio_rb32(pb); /* minor version */
>      av_dict_set_int(&c->fc->metadata, "minor_version", minor_ver, 0);
>  
> -    comp_brand_size = atom.size - 8;
> -    if (comp_brand_size < 0 || comp_brand_size == INT_MAX)
> +    comp_brand_count = (atom.size - 8) / 4;
> +    if (!comp_brand_count)
> +        return 0;
> +    else if (comp_brand_count < 0 || comp_brand_count > INT_MAX / 5)
>          return AVERROR_INVALIDDATA;
> -    comp_brands_str = av_malloc(comp_brand_size + 1); /* Add null terminator */
> +    /* add separator between multiple brands, add null terminator */
> +    comp_brands_str = av_malloc(comp_brand_count * 5);
>      if (!comp_brands_str)
>          return AVERROR(ENOMEM);
>  
> -    ret = ffio_read_size(pb, comp_brands_str, comp_brand_size);
> -    if (ret < 0) {
> -        av_freep(&comp_brands_str);
> -        return ret;
> +    for (int i = 0; i < comp_brand_count; i++) {
> +        ret = ffio_read_size(pb, comp_brands_str + i * 5, 4);
> +        if (ret < 0) {
> +            av_freep(&comp_brands_str);
> +            return ret;
> +        }
> +        comp_brands_str[i * 5 + 4] = ',';
>      }
> -    comp_brands_str[comp_brand_size] = 0;
> +    comp_brands_str[comp_brand_count * 5 - 1] = '\0';
>      av_dict_set(&c->fc->metadata, "compatible_brands",
>                  comp_brands_str, AV_DICT_DONT_STRDUP_VAL);
>  
>
Zhao Zhili Sept. 9, 2020, 2:32 p.m. UTC | #3
> On Sep 9, 2020, at 9:51 PM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
> 
> On 09/09/2020 14:07, quinkblack@foxmail.com wrote:
>> From: Zhao Zhili <quinkblack@foxmail.com>
>> 
>> Use comma as separator between multiple compatible brands.
>> ---
>> libavformat/mov.c | 24 +++++++++++++++---------
>> 1 file changed, 15 insertions(+), 9 deletions(-)
> 
> This change appears to actually be several dfferent changes in one,
> most of which are not listed in the commit message.

Do you mean return early when compatible brands is empty?

> 
> - 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".
Zhao Zhili Sept. 9, 2020, 2:52 p.m. UTC | #4
> On Sep 9, 2020, at 10:24 PM, James Almer <jamrial@gmail.com> wrote:
> 
> On 9/9/2020 10:07 AM, quinkblack@foxmail.com wrote:
>> From: Zhao Zhili <quinkblack@foxmail.com>
>> 
>> Use comma as separator between multiple compatible brands.
> 
> Wont this potentially break parsing of the output of ffmpeg/ffprobe, or
> even of the compatible_brands key in c->fc->metadata when using the dict
> API?

The format of metadata value is not documented API, it's fail enough
to say the user may depend on the undefined behavior.

After a second thought, is it safe enough to change be output of
ffmpeg/ffprobe but keep c->fc->metadata unmodified?

> 
>> ---
>> libavformat/mov.c | 24 +++++++++++++++---------
>> 1 file changed, 15 insertions(+), 9 deletions(-)
>> 
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 690beb10ce..8f5341f925 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -1093,7 +1093,7 @@ static int aax_filter(uint8_t *input, int size, MOVContext *c)
>> static int mov_read_ftyp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> {
>>     uint32_t minor_ver;
>> -    int comp_brand_size;
>> +    int comp_brand_count;
>>     char* comp_brands_str;
>>     uint8_t type[5] = {0};
>>     int ret = ffio_read_size(pb, type, 4);
>> @@ -1107,19 +1107,25 @@ static int mov_read_ftyp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>     minor_ver = avio_rb32(pb); /* minor version */
>>     av_dict_set_int(&c->fc->metadata, "minor_version", minor_ver, 0);
>> 
>> -    comp_brand_size = atom.size - 8;
>> -    if (comp_brand_size < 0 || comp_brand_size == INT_MAX)
>> +    comp_brand_count = (atom.size - 8) / 4;
>> +    if (!comp_brand_count)
>> +        return 0;
>> +    else if (comp_brand_count < 0 || comp_brand_count > INT_MAX / 5)
>>         return AVERROR_INVALIDDATA;
>> -    comp_brands_str = av_malloc(comp_brand_size + 1); /* Add null terminator */
>> +    /* add separator between multiple brands, add null terminator */
>> +    comp_brands_str = av_malloc(comp_brand_count * 5);
>>     if (!comp_brands_str)
>>         return AVERROR(ENOMEM);
>> 
>> -    ret = ffio_read_size(pb, comp_brands_str, comp_brand_size);
>> -    if (ret < 0) {
>> -        av_freep(&comp_brands_str);
>> -        return ret;
>> +    for (int i = 0; i < comp_brand_count; i++) {
>> +        ret = ffio_read_size(pb, comp_brands_str + i * 5, 4);
>> +        if (ret < 0) {
>> +            av_freep(&comp_brands_str);
>> +            return ret;
>> +        }
>> +        comp_brands_str[i * 5 + 4] = ',';
>>     }
>> -    comp_brands_str[comp_brand_size] = 0;
>> +    comp_brands_str[comp_brand_count * 5 - 1] = '\0';
>>     av_dict_set(&c->fc->metadata, "compatible_brands",
>>                 comp_brands_str, AV_DICT_DONT_STRDUP_VAL);
>> 
>> 
> 
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 690beb10ce..8f5341f925 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1093,7 +1093,7 @@  static int aax_filter(uint8_t *input, int size, MOVContext *c)
 static int mov_read_ftyp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
     uint32_t minor_ver;
-    int comp_brand_size;
+    int comp_brand_count;
     char* comp_brands_str;
     uint8_t type[5] = {0};
     int ret = ffio_read_size(pb, type, 4);
@@ -1107,19 +1107,25 @@  static int mov_read_ftyp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     minor_ver = avio_rb32(pb); /* minor version */
     av_dict_set_int(&c->fc->metadata, "minor_version", minor_ver, 0);
 
-    comp_brand_size = atom.size - 8;
-    if (comp_brand_size < 0 || comp_brand_size == INT_MAX)
+    comp_brand_count = (atom.size - 8) / 4;
+    if (!comp_brand_count)
+        return 0;
+    else if (comp_brand_count < 0 || comp_brand_count > INT_MAX / 5)
         return AVERROR_INVALIDDATA;
-    comp_brands_str = av_malloc(comp_brand_size + 1); /* Add null terminator */
+    /* add separator between multiple brands, add null terminator */
+    comp_brands_str = av_malloc(comp_brand_count * 5);
     if (!comp_brands_str)
         return AVERROR(ENOMEM);
 
-    ret = ffio_read_size(pb, comp_brands_str, comp_brand_size);
-    if (ret < 0) {
-        av_freep(&comp_brands_str);
-        return ret;
+    for (int i = 0; i < comp_brand_count; i++) {
+        ret = ffio_read_size(pb, comp_brands_str + i * 5, 4);
+        if (ret < 0) {
+            av_freep(&comp_brands_str);
+            return ret;
+        }
+        comp_brands_str[i * 5 + 4] = ',';
     }
-    comp_brands_str[comp_brand_size] = 0;
+    comp_brands_str[comp_brand_count * 5 - 1] = '\0';
     av_dict_set(&c->fc->metadata, "compatible_brands",
                 comp_brands_str, AV_DICT_DONT_STRDUP_VAL);