| Message ID | tencent_D693399C714B94261050A326F4E7C1EA8406@qq.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,major,bump,1/6] libavutil/hdr_dynamic_vivid_metadata: fix AVHDRVividColorToneMappingParams | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/commit_msg_loongarch64 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/commit_msg_x86 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting Zhao Zhili (2023-02-02 08:02:03) > From: Zhao Zhili <zhilizhao@tencent.com> > > There are two group of three_Spline params. Fix the struct > definition and usecases inside libavcodec, libavfilter and ffprobe. > > Co-Author: Houxiang ZHU <zhuhouxiang@theuwa.com> > Signed-off-by: Zhao Zhili <zhilizhao@tencent.com> > --- > diff --git a/libavutil/hdr_dynamic_vivid_metadata.h b/libavutil/hdr_dynamic_vivid_metadata.h > index a34f83072c..4ceddc539d 100644 > --- a/libavutil/hdr_dynamic_vivid_metadata.h > +++ b/libavutil/hdr_dynamic_vivid_metadata.h > @@ -126,42 +126,42 @@ typedef struct AVHDRVividColorToneMappingParams { > * The mode of three Spline. the value shall be in the range > * of 0 to 3, inclusive. > */ > - int three_Spline_TH_mode; > + int three_Spline_TH_mode[2]; > > /** > * three_Spline_TH_enable_MB is in the range of 0.0 to 1.0, inclusive > * and in multiples of 1.0/255. > * > */ > - AVRational three_Spline_TH_enable_MB; > + AVRational three_Spline_TH_enable_MB[2]; > > /** > * 3Spline_TH_enable of three Spline. > * The value shall be in the range of 0.0 to 1.0, inclusive. > * and in multiples of 1.0/4095. > */ > - AVRational three_Spline_TH_enable; > + AVRational three_Spline_TH_enable[2]; > > /** > * 3Spline_TH_Delta1 of three Spline. > * The value shall be in the range of 0.0 to 0.25, inclusive, > * and in multiples of 0.25/1023. > */ > - AVRational three_Spline_TH_Delta1; > + AVRational three_Spline_TH_Delta1[2]; > > /** > * 3Spline_TH_Delta2 of three Spline. > * The value shall be in the range of 0.0 to 0.25, inclusive, > * and in multiples of 0.25/1023. > */ > - AVRational three_Spline_TH_Delta2; > + AVRational three_Spline_TH_Delta2[2]; > > /** > * 3Spline_enable_Strength of three Spline. > * The value shall be in the range of 0.0 to 1.0, inclusive, > * and in multiples of 1.0/255. > */ > - AVRational three_Spline_enable_Strength; > + AVRational three_Spline_enable_Strength[2]; > } AVHDRVividColorToneMappingParams; A major bump is not for breaking APIs however you like, only for things that were scheduled in advance that our callers could have prepared for. You should add new fields, not change existing ones. Also, the documentation and doc/APIchanges need to be updated.
> On Feb 2, 2023, at 16:16, Anton Khirnov <anton@khirnov.net> wrote: > > Quoting Zhao Zhili (2023-02-02 08:02:03) >> From: Zhao Zhili <zhilizhao@tencent.com> >> >> There are two group of three_Spline params. Fix the struct >> definition and usecases inside libavcodec, libavfilter and ffprobe. >> >> Co-Author: Houxiang ZHU <zhuhouxiang@theuwa.com> >> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com> >> --- >> diff --git a/libavutil/hdr_dynamic_vivid_metadata.h b/libavutil/hdr_dynamic_vivid_metadata.h >> index a34f83072c..4ceddc539d 100644 >> --- a/libavutil/hdr_dynamic_vivid_metadata.h >> +++ b/libavutil/hdr_dynamic_vivid_metadata.h >> @@ -126,42 +126,42 @@ typedef struct AVHDRVividColorToneMappingParams { >> * The mode of three Spline. the value shall be in the range >> * of 0 to 3, inclusive. >> */ >> - int three_Spline_TH_mode; >> + int three_Spline_TH_mode[2]; >> >> /** >> * three_Spline_TH_enable_MB is in the range of 0.0 to 1.0, inclusive >> * and in multiples of 1.0/255. >> * >> */ >> - AVRational three_Spline_TH_enable_MB; >> + AVRational three_Spline_TH_enable_MB[2]; >> >> /** >> * 3Spline_TH_enable of three Spline. >> * The value shall be in the range of 0.0 to 1.0, inclusive. >> * and in multiples of 1.0/4095. >> */ >> - AVRational three_Spline_TH_enable; >> + AVRational three_Spline_TH_enable[2]; >> >> /** >> * 3Spline_TH_Delta1 of three Spline. >> * The value shall be in the range of 0.0 to 0.25, inclusive, >> * and in multiples of 0.25/1023. >> */ >> - AVRational three_Spline_TH_Delta1; >> + AVRational three_Spline_TH_Delta1[2]; >> >> /** >> * 3Spline_TH_Delta2 of three Spline. >> * The value shall be in the range of 0.0 to 0.25, inclusive, >> * and in multiples of 0.25/1023. >> */ >> - AVRational three_Spline_TH_Delta2; >> + AVRational three_Spline_TH_Delta2[2]; >> >> /** >> * 3Spline_enable_Strength of three Spline. >> * The value shall be in the range of 0.0 to 1.0, inclusive, >> * and in multiples of 1.0/255. >> */ >> - AVRational three_Spline_enable_Strength; >> + AVRational three_Spline_enable_Strength[2]; >> } AVHDRVividColorToneMappingParams; > > A major bump is not for breaking APIs however you like, only for things > that were scheduled in advance that our callers could have prepared for. > You should add new fields, not change existing ones. > Also, the documentation and doc/APIchanges need to be updated. Adding new fields works, but very ugly. The code never work if three_Spline_num > 1. Breaking API to let user notice that may not be a bad thing in this specific situation. > > -- > Anton Khirnov > _______________________________________________ > 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".
Quoting zhilizhao(赵志立) (2023-02-02 09:52:43) > > On Feb 2, 2023, at 16:16, Anton Khirnov <anton@khirnov.net> wrote: > > > > A major bump is not for breaking APIs however you like, only for things > > that were scheduled in advance that our callers could have prepared for. > > You should add new fields, not change existing ones. > > Also, the documentation and doc/APIchanges need to be updated. > > Adding new fields works, but very ugly. > > The code never work if three_Spline_num > 1. Breaking API to let user > notice that may not be a bad thing in this specific situation. Sorry, I disagree. If it could have worked at all - and IIUC it did when three_Spline_num == 1 - then we should maintain compatibility. The deprecation notice will notify developers just as well, while compilation breakage just annoys users who are unable to fix it. Constant compilation breakage on upgrading to new major versions is one of the main reasons many of our users bundle old versions of our libraries with their code. We should avoid unscheduled breakage whenever possible.
diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c index dfa7ff1b24..a853c70f56 100644 --- a/fftools/ffprobe.c +++ b/fftools/ffprobe.c @@ -2246,14 +2246,16 @@ static void print_dynamic_hdr_vivid(WriterContext *w, const AVDynamicHDRVivid *m print_int("3Spline_enable_flag", tm_params->three_Spline_enable_flag); if (tm_params->three_Spline_enable_flag) { print_int("3Spline_num", tm_params->three_Spline_num); - print_int("3Spline_TH_mode", tm_params->three_Spline_TH_mode); for (int j = 0; j < tm_params->three_Spline_num; j++) { - print_q("3Spline_TH_enable_MB", tm_params->three_Spline_TH_enable_MB, '/'); - print_q("3Spline_TH_enable", tm_params->three_Spline_TH_enable, '/'); - print_q("3Spline_TH_Delta1", tm_params->three_Spline_TH_Delta1, '/'); - print_q("3Spline_TH_Delta2", tm_params->three_Spline_TH_Delta2, '/'); - print_q("3Spline_enable_Strength", tm_params->three_Spline_enable_Strength, '/'); + print_int("3Spline_TH_mode", tm_params->three_Spline_TH_mode[j]); + if (tm_params->three_Spline_TH_mode[j] == 0 || tm_params->three_Spline_TH_mode[j] == 2) { + print_q("3Spline_TH_enable_MB", tm_params->three_Spline_TH_enable_MB[j], '/'); + } + print_q("3Spline_TH_enable", tm_params->three_Spline_TH_enable[j], '/'); + print_q("3Spline_TH_Delta1", tm_params->three_Spline_TH_Delta1[j], '/'); + print_q("3Spline_TH_Delta2", tm_params->three_Spline_TH_Delta2[j], '/'); + print_q("3Spline_enable_Strength", tm_params->three_Spline_enable_Strength[j], '/'); } } } diff --git a/libavcodec/dynamic_hdr_vivid.c b/libavcodec/dynamic_hdr_vivid.c index d689669dec..f7a41ed2d5 100644 --- a/libavcodec/dynamic_hdr_vivid.c +++ b/libavcodec/dynamic_hdr_vivid.c @@ -101,23 +101,21 @@ int ff_parse_itu_t_t35_to_dynamic_hdr_vivid(AVDynamicHDRVivid *s, const uint8_t if (get_bits_left(gb) < 1 + tm_params->three_Spline_num * (2 + 12 + 28 + 1)) return AVERROR_INVALIDDATA; tm_params->three_Spline_num = get_bits(gb, 1) + 1; + if (tm_params->three_Spline_num > FF_ARRAY_ELEMS(tm_params->three_Spline_TH_mode)) + return AVERROR_INVALIDDATA; for (int j = 0; j < tm_params->three_Spline_num; j++) { - tm_params->three_Spline_TH_mode = get_bits(gb, 2); - if (tm_params->three_Spline_TH_mode == 0 || tm_params->three_Spline_TH_mode == 2) { + tm_params->three_Spline_TH_mode[j] = get_bits(gb, 2); + if (tm_params->three_Spline_TH_mode[j] == 0 || tm_params->three_Spline_TH_mode[j] == 2) { if (get_bits_left(gb) < 8) return AVERROR_INVALIDDATA; - tm_params->three_Spline_TH_enable_MB = (AVRational){get_bits(gb, 8), 255}; + tm_params->three_Spline_TH_enable_MB[j] = (AVRational){get_bits(gb, 8), 255}; } - tm_params->three_Spline_TH_enable = (AVRational){get_bits(gb, 12), 4095}; - tm_params->three_Spline_TH_Delta1 = (AVRational){get_bits(gb, 10), 1023}; - tm_params->three_Spline_TH_Delta2 = (AVRational){get_bits(gb, 10), 1023}; - tm_params->three_Spline_enable_Strength = (AVRational){get_bits(gb, 8), 255}; + tm_params->three_Spline_TH_enable[j] = (AVRational){get_bits(gb, 12), 4095}; + tm_params->three_Spline_TH_Delta1[j] = (AVRational){get_bits(gb, 10), 1023}; + tm_params->three_Spline_TH_Delta2[j] = (AVRational){get_bits(gb, 10), 1023}; + tm_params->three_Spline_enable_Strength[j] = (AVRational){get_bits(gb, 8), 255}; } - } else { - tm_params->three_Spline_num = 1; - tm_params->three_Spline_TH_mode = 0; } - } } } diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c index e55625b338..05829289a5 100644 --- a/libavfilter/vf_showinfo.c +++ b/libavfilter/vf_showinfo.c @@ -354,19 +354,20 @@ static void dump_dynamic_hdr_vivid(AVFilterContext *ctx, AVFrameSideData *sd) av_log(ctx, AV_LOG_INFO, "3Spline_enable_flag[%d][%d]: %d, ", w, i, tm_params->three_Spline_enable_flag); if (tm_params->three_Spline_enable_flag) { - av_log(ctx, AV_LOG_INFO, "3Spline_TH_mode[%d][%d]: %d, ", w, i, tm_params->three_Spline_TH_mode); - for (int j = 0; j < tm_params->three_Spline_num; j++) { - av_log(ctx, AV_LOG_INFO, "3Spline_TH_enable_MB[%d][%d][%d]: %.4f, ", - w, i, j, av_q2d(tm_params->three_Spline_TH_enable_MB)); + av_log(ctx, AV_LOG_INFO, "3Spline_TH_mode[%d][%d]: %d, ", w, i, tm_params->three_Spline_TH_mode[j]); + if (tm_params->three_Spline_TH_mode[j] == 0 || tm_params->three_Spline_TH_mode[j] == 2) { + av_log(ctx, AV_LOG_INFO, "3Spline_TH_enable_MB[%d][%d][%d]: %.4f, ", + w, i, j, av_q2d(tm_params->three_Spline_TH_enable_MB[j])); + } av_log(ctx, AV_LOG_INFO, "3Spline_TH_enable[%d][%d][%d]: %.4f, ", - w, i, j, av_q2d(tm_params->three_Spline_TH_enable)); + w, i, j, av_q2d(tm_params->three_Spline_TH_enable[j])); av_log(ctx, AV_LOG_INFO, "3Spline_TH_Delta1[%d][%d][%d]: %.4f, ", - w, i, j, av_q2d(tm_params->three_Spline_TH_Delta1)); + w, i, j, av_q2d(tm_params->three_Spline_TH_Delta1[j])); av_log(ctx, AV_LOG_INFO, "3Spline_TH_Delta2[%d][%d][%d]: %.4f, ", - w, i, j, av_q2d(tm_params->three_Spline_TH_Delta2)); + w, i, j, av_q2d(tm_params->three_Spline_TH_Delta2[j])); av_log(ctx, AV_LOG_INFO, "3Spline_enable_Strength[%d][%d][%d]: %.4f, ", - w, i, j, av_q2d(tm_params->three_Spline_enable_Strength)); + w, i, j, av_q2d(tm_params->three_Spline_enable_Strength[j])); } } } diff --git a/libavutil/hdr_dynamic_vivid_metadata.h b/libavutil/hdr_dynamic_vivid_metadata.h index a34f83072c..4ceddc539d 100644 --- a/libavutil/hdr_dynamic_vivid_metadata.h +++ b/libavutil/hdr_dynamic_vivid_metadata.h @@ -126,42 +126,42 @@ typedef struct AVHDRVividColorToneMappingParams { * The mode of three Spline. the value shall be in the range * of 0 to 3, inclusive. */ - int three_Spline_TH_mode; + int three_Spline_TH_mode[2]; /** * three_Spline_TH_enable_MB is in the range of 0.0 to 1.0, inclusive * and in multiples of 1.0/255. * */ - AVRational three_Spline_TH_enable_MB; + AVRational three_Spline_TH_enable_MB[2]; /** * 3Spline_TH_enable of three Spline. * The value shall be in the range of 0.0 to 1.0, inclusive. * and in multiples of 1.0/4095. */ - AVRational three_Spline_TH_enable; + AVRational three_Spline_TH_enable[2]; /** * 3Spline_TH_Delta1 of three Spline. * The value shall be in the range of 0.0 to 0.25, inclusive, * and in multiples of 0.25/1023. */ - AVRational three_Spline_TH_Delta1; + AVRational three_Spline_TH_Delta1[2]; /** * 3Spline_TH_Delta2 of three Spline. * The value shall be in the range of 0.0 to 0.25, inclusive, * and in multiples of 0.25/1023. */ - AVRational three_Spline_TH_Delta2; + AVRational three_Spline_TH_Delta2[2]; /** * 3Spline_enable_Strength of three Spline. * The value shall be in the range of 0.0 to 1.0, inclusive, * and in multiples of 1.0/255. */ - AVRational three_Spline_enable_Strength; + AVRational three_Spline_enable_Strength[2]; } AVHDRVividColorToneMappingParams;