| Message ID | tencent_F23E5956819D96E0445AE835958106349F09@qq.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mov: make compatible brands more readable | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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
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); > >
> 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".
> 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 --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);
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(-)