| Message ID | 20190522010441.44257-4-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 22/05/2019 02:04, Andreas Rheinhardt wrote: > If a sequence display extension is read with colour_description equal to > zero, but a user wants to add one or more of the colour_description > elements, then the colour_description elements the user did not explicitly > request to be set are set to zero and not to the value equal to > unknown/unspecified (namely 2). A value of zero is not only inappropriate, > but explicitly forbidden. This is fixed by inferring the right default > values during the reading process if the elements are absent; moreover, > changing any of the colour_description elements to zero is now no longer > permitted. > > Furthermore, if a sequence display extension has to be added, the > earlier code set some fields to their default value twice. This has been > changed, too. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/cbs_mpeg2.c | 15 +++++++++++++++ > libavcodec/cbs_mpeg2_syntax_template.c | 4 ++++ > libavcodec/mpeg2_metadata_bsf.c | 18 ++++++++++++------ > 3 files changed, 31 insertions(+), 6 deletions(-) > > diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c > index 1d319e0947..437eac88a3 100644 > --- a/libavcodec/cbs_mpeg2.c > +++ b/libavcodec/cbs_mpeg2.c > @@ -71,6 +71,10 @@ > (get_bits_left(rw) >= width && \ > (var = show_bits(rw, width)) == (compare)) > > +#define infer(name, value) do { \ > + current->name = value; \ > + } while (0) > + > #include "cbs_mpeg2_syntax_template.c" > > #undef READ > @@ -79,6 +83,7 @@ > #undef xui > #undef marker_bit > #undef nextbits > +#undef infer > > > #define WRITE > @@ -97,6 +102,15 @@ > > #define nextbits(width, compare, var) (var) > > +#define infer(name, value) do { \ > + if (current->name != (value)) { \ > + av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \ > + "%s does not match inferred value: " \ > + "%"PRId64", but should be %"PRId64".\n", \ > + #name, (int64_t)current->name, (int64_t)(value)); \ > + } \ > + } while (0) > + > #include "cbs_mpeg2_syntax_template.c" > > #undef READ > @@ -105,6 +119,7 @@ > #undef xui > #undef marker_bit > #undef nextbits > +#undef infer > > > static void cbs_mpeg2_free_user_data(void *unit, uint8_t *content) > diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c > index b9d53682fe..87db0ad039 100644 > --- a/libavcodec/cbs_mpeg2_syntax_template.c > +++ b/libavcodec/cbs_mpeg2_syntax_template.c > @@ -144,6 +144,10 @@ static int FUNC(sequence_display_extension)(CodedBitstreamContext *ctx, RWContex > uir(8, transfer_characteristics); > uir(8, matrix_coefficients); > #endif > + } else { > + infer(colour_primaries, 2); > + infer(transfer_characteristics, 2); > + infer(matrix_coefficients, 2); > } > > ui(14, display_horizontal_size); > diff --git a/libavcodec/mpeg2_metadata_bsf.c b/libavcodec/mpeg2_metadata_bsf.c > index ba3a74afda..5aed41a008 100644 > --- a/libavcodec/mpeg2_metadata_bsf.c > +++ b/libavcodec/mpeg2_metadata_bsf.c > @@ -147,18 +147,12 @@ static int mpeg2_metadata_update_fragment(AVBSFContext *bsf, > > if (ctx->colour_primaries >= 0) > sde->colour_primaries = ctx->colour_primaries; > - else if (add_sde) > - sde->colour_primaries = 2; > > if (ctx->transfer_characteristics >= 0) > sde->transfer_characteristics = ctx->transfer_characteristics; > - else if (add_sde) > - sde->transfer_characteristics = 2; > > if (ctx->matrix_coefficients >= 0) > sde->matrix_coefficients = ctx->matrix_coefficients; > - else if (add_sde) > - sde->matrix_coefficients = 2; > } > } > > @@ -229,6 +223,18 @@ static int mpeg2_metadata_init(AVBSFContext *bsf) > CodedBitstreamFragment *frag = &ctx->fragment; > int err; > > +#define VALIDITY_CHECK(name) do { \ > + if (!ctx->name) { \ > + av_log(bsf, AV_LOG_ERROR, "The value 0 for %s is " \ > + "forbidden.\n", #name); \ > + return AVERROR(EINVAL); \ > + } \ > + } while (0) > + VALIDITY_CHECK(colour_primaries); > + VALIDITY_CHECK(transfer_characteristics); > + VALIDITY_CHECK(matrix_coefficients); > +#undef VALIDITY_CHECK Perhaps we could use the normal option checking to enforce this? Suppose we change the range from [-1, 255] to [0, 255] and make the do-nothing default value 0. Then the checks above become ctx->colour_primaries > 0 and this separate test is not needed. > + > err = ff_cbs_init(&ctx->cbc, AV_CODEC_ID_MPEG2VIDEO, bsf); > if (err < 0) > return err; > - Mark
Mark Thompson: > On 22/05/2019 02:04, Andreas Rheinhardt wrote: >> If a sequence display extension is read with colour_description equal to >> zero, but a user wants to add one or more of the colour_description >> elements, then the colour_description elements the user did not explicitly >> request to be set are set to zero and not to the value equal to >> unknown/unspecified (namely 2). A value of zero is not only inappropriate, >> but explicitly forbidden. This is fixed by inferring the right default >> values during the reading process if the elements are absent; moreover, >> changing any of the colour_description elements to zero is now no longer >> permitted. >> >> Furthermore, if a sequence display extension has to be added, the >> earlier code set some fields to their default value twice. This has been >> changed, too. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavcodec/cbs_mpeg2.c | 15 +++++++++++++++ >> libavcodec/cbs_mpeg2_syntax_template.c | 4 ++++ >> libavcodec/mpeg2_metadata_bsf.c | 18 ++++++++++++------ >> 3 files changed, 31 insertions(+), 6 deletions(-) >> >> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c >> index 1d319e0947..437eac88a3 100644 >> --- a/libavcodec/cbs_mpeg2.c >> +++ b/libavcodec/cbs_mpeg2.c >> @@ -71,6 +71,10 @@ >> (get_bits_left(rw) >= width && \ >> (var = show_bits(rw, width)) == (compare)) >> >> +#define infer(name, value) do { \ >> + current->name = value; \ >> + } while (0) >> + >> #include "cbs_mpeg2_syntax_template.c" >> >> #undef READ >> @@ -79,6 +83,7 @@ >> #undef xui >> #undef marker_bit >> #undef nextbits >> +#undef infer >> >> >> #define WRITE >> @@ -97,6 +102,15 @@ >> >> #define nextbits(width, compare, var) (var) >> >> +#define infer(name, value) do { \ >> + if (current->name != (value)) { \ >> + av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \ >> + "%s does not match inferred value: " \ >> + "%"PRId64", but should be %"PRId64".\n", \ >> + #name, (int64_t)current->name, (int64_t)(value)); \ >> + } \ >> + } while (0) >> + >> #include "cbs_mpeg2_syntax_template.c" >> >> #undef READ >> @@ -105,6 +119,7 @@ >> #undef xui >> #undef marker_bit >> #undef nextbits >> +#undef infer >> >> >> static void cbs_mpeg2_free_user_data(void *unit, uint8_t *content) >> diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c >> index b9d53682fe..87db0ad039 100644 >> --- a/libavcodec/cbs_mpeg2_syntax_template.c >> +++ b/libavcodec/cbs_mpeg2_syntax_template.c >> @@ -144,6 +144,10 @@ static int FUNC(sequence_display_extension)(CodedBitstreamContext *ctx, RWContex >> uir(8, transfer_characteristics); >> uir(8, matrix_coefficients); >> #endif >> + } else { >> + infer(colour_primaries, 2); >> + infer(transfer_characteristics, 2); >> + infer(matrix_coefficients, 2); >> } >> >> ui(14, display_horizontal_size); >> diff --git a/libavcodec/mpeg2_metadata_bsf.c b/libavcodec/mpeg2_metadata_bsf.c >> index ba3a74afda..5aed41a008 100644 >> --- a/libavcodec/mpeg2_metadata_bsf.c >> +++ b/libavcodec/mpeg2_metadata_bsf.c >> @@ -147,18 +147,12 @@ static int mpeg2_metadata_update_fragment(AVBSFContext *bsf, >> >> if (ctx->colour_primaries >= 0) >> sde->colour_primaries = ctx->colour_primaries; >> - else if (add_sde) >> - sde->colour_primaries = 2; >> >> if (ctx->transfer_characteristics >= 0) >> sde->transfer_characteristics = ctx->transfer_characteristics; >> - else if (add_sde) >> - sde->transfer_characteristics = 2; >> >> if (ctx->matrix_coefficients >= 0) >> sde->matrix_coefficients = ctx->matrix_coefficients; >> - else if (add_sde) >> - sde->matrix_coefficients = 2; >> } >> } >> >> @@ -229,6 +223,18 @@ static int mpeg2_metadata_init(AVBSFContext *bsf) >> CodedBitstreamFragment *frag = &ctx->fragment; >> int err; >> >> +#define VALIDITY_CHECK(name) do { \ >> + if (!ctx->name) { \ >> + av_log(bsf, AV_LOG_ERROR, "The value 0 for %s is " \ >> + "forbidden.\n", #name); \ >> + return AVERROR(EINVAL); \ >> + } \ >> + } while (0) >> + VALIDITY_CHECK(colour_primaries); >> + VALIDITY_CHECK(transfer_characteristics); >> + VALIDITY_CHECK(matrix_coefficients); >> +#undef VALIDITY_CHECK > > Perhaps we could use the normal option checking to enforce this? Suppose we change the range from [-1, 255] to [0, 255] and make the do-nothing default value 0. Then the checks above become ctx->colour_primaries > 0 and this separate test is not needed. > There are two reasons why I didn't do this: 1. A user with only a background in (say) H.264 or H.265 might want to put a value of 0 in there, thinking it is a legel value. Said user is going to be surprised. 2. I have not given up hope on updating the AVCodecParameters (do you remember https://ffmpeg.org/pipermail/ffmpeg-devel/2018-December/236990.html ?) and if 0 means do nothing, then it is not in line with the rest of cbs and needs special handling there. But both reasons are very weak and so I will consider your suggestion. >> + >> err = ff_cbs_init(&ctx->cbc, AV_CODEC_ID_MPEG2VIDEO, bsf); >> if (err < 0) >> return err; >> > > - Mark > _______________________________________________ > 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 29/05/2019 05:12, Andreas Rheinhardt wrote: > Mark Thompson: >> On 22/05/2019 02:04, Andreas Rheinhardt wrote: >>> If a sequence display extension is read with colour_description equal to >>> zero, but a user wants to add one or more of the colour_description >>> elements, then the colour_description elements the user did not explicitly >>> request to be set are set to zero and not to the value equal to >>> unknown/unspecified (namely 2). A value of zero is not only inappropriate, >>> but explicitly forbidden. This is fixed by inferring the right default >>> values during the reading process if the elements are absent; moreover, >>> changing any of the colour_description elements to zero is now no longer >>> permitted. >>> >>> Furthermore, if a sequence display extension has to be added, the >>> earlier code set some fields to their default value twice. This has been >>> changed, too. >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> libavcodec/cbs_mpeg2.c | 15 +++++++++++++++ >>> libavcodec/cbs_mpeg2_syntax_template.c | 4 ++++ >>> libavcodec/mpeg2_metadata_bsf.c | 18 ++++++++++++------ >>> 3 files changed, 31 insertions(+), 6 deletions(-) >>> >>> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c >>> index 1d319e0947..437eac88a3 100644 >>> --- a/libavcodec/cbs_mpeg2.c >>> +++ b/libavcodec/cbs_mpeg2.c >>> @@ -71,6 +71,10 @@ >>> (get_bits_left(rw) >= width && \ >>> (var = show_bits(rw, width)) == (compare)) >>> >>> +#define infer(name, value) do { \ >>> + current->name = value; \ >>> + } while (0) >>> + >>> #include "cbs_mpeg2_syntax_template.c" >>> >>> #undef READ >>> @@ -79,6 +83,7 @@ >>> #undef xui >>> #undef marker_bit >>> #undef nextbits >>> +#undef infer >>> >>> >>> #define WRITE >>> @@ -97,6 +102,15 @@ >>> >>> #define nextbits(width, compare, var) (var) >>> >>> +#define infer(name, value) do { \ >>> + if (current->name != (value)) { \ >>> + av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \ >>> + "%s does not match inferred value: " \ >>> + "%"PRId64", but should be %"PRId64".\n", \ >>> + #name, (int64_t)current->name, (int64_t)(value)); \ >>> + } \ >>> + } while (0) >>> + >>> #include "cbs_mpeg2_syntax_template.c" >>> >>> #undef READ >>> @@ -105,6 +119,7 @@ >>> #undef xui >>> #undef marker_bit >>> #undef nextbits >>> +#undef infer >>> >>> >>> static void cbs_mpeg2_free_user_data(void *unit, uint8_t *content) >>> diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c >>> index b9d53682fe..87db0ad039 100644 >>> --- a/libavcodec/cbs_mpeg2_syntax_template.c >>> +++ b/libavcodec/cbs_mpeg2_syntax_template.c >>> @@ -144,6 +144,10 @@ static int FUNC(sequence_display_extension)(CodedBitstreamContext *ctx, RWContex >>> uir(8, transfer_characteristics); >>> uir(8, matrix_coefficients); >>> #endif >>> + } else { >>> + infer(colour_primaries, 2); >>> + infer(transfer_characteristics, 2); >>> + infer(matrix_coefficients, 2); >>> } >>> >>> ui(14, display_horizontal_size); >>> diff --git a/libavcodec/mpeg2_metadata_bsf.c b/libavcodec/mpeg2_metadata_bsf.c >>> index ba3a74afda..5aed41a008 100644 >>> --- a/libavcodec/mpeg2_metadata_bsf.c >>> +++ b/libavcodec/mpeg2_metadata_bsf.c >>> @@ -147,18 +147,12 @@ static int mpeg2_metadata_update_fragment(AVBSFContext *bsf, >>> >>> if (ctx->colour_primaries >= 0) >>> sde->colour_primaries = ctx->colour_primaries; >>> - else if (add_sde) >>> - sde->colour_primaries = 2; >>> >>> if (ctx->transfer_characteristics >= 0) >>> sde->transfer_characteristics = ctx->transfer_characteristics; >>> - else if (add_sde) >>> - sde->transfer_characteristics = 2; >>> >>> if (ctx->matrix_coefficients >= 0) >>> sde->matrix_coefficients = ctx->matrix_coefficients; >>> - else if (add_sde) >>> - sde->matrix_coefficients = 2; >>> } >>> } >>> >>> @@ -229,6 +223,18 @@ static int mpeg2_metadata_init(AVBSFContext *bsf) >>> CodedBitstreamFragment *frag = &ctx->fragment; >>> int err; >>> >>> +#define VALIDITY_CHECK(name) do { \ >>> + if (!ctx->name) { \ >>> + av_log(bsf, AV_LOG_ERROR, "The value 0 for %s is " \ >>> + "forbidden.\n", #name); \ >>> + return AVERROR(EINVAL); \ >>> + } \ >>> + } while (0) >>> + VALIDITY_CHECK(colour_primaries); >>> + VALIDITY_CHECK(transfer_characteristics); >>> + VALIDITY_CHECK(matrix_coefficients); >>> +#undef VALIDITY_CHECK >> >> Perhaps we could use the normal option checking to enforce this? Suppose we change the range from [-1, 255] to [0, 255] and make the do-nothing default value 0. Then the checks above become ctx->colour_primaries > 0 and this separate test is not needed. >> > There are two reasons why I didn't do this: > 1. A user with only a background in (say) H.264 or H.265 might want to > put a value of 0 in there, thinking it is a legel value. Said user is > going to be surprised. > 2. I have not given up hope on updating the AVCodecParameters (do you > remember > https://ffmpeg.org/pipermail/ffmpeg-devel/2018-December/236990.html ?) > and if 0 means do nothing, then it is not in line with the rest of cbs > and needs special handling there. > But both reasons are very weak and so I will consider your suggestion. Fair enough. I have no actual objection to your approach; it was mostly a thought that the code could be slightly simpler. Thanks, - Mark
diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c index 1d319e0947..437eac88a3 100644 --- a/libavcodec/cbs_mpeg2.c +++ b/libavcodec/cbs_mpeg2.c @@ -71,6 +71,10 @@ (get_bits_left(rw) >= width && \ (var = show_bits(rw, width)) == (compare)) +#define infer(name, value) do { \ + current->name = value; \ + } while (0) + #include "cbs_mpeg2_syntax_template.c" #undef READ @@ -79,6 +83,7 @@ #undef xui #undef marker_bit #undef nextbits +#undef infer #define WRITE @@ -97,6 +102,15 @@ #define nextbits(width, compare, var) (var) +#define infer(name, value) do { \ + if (current->name != (value)) { \ + av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \ + "%s does not match inferred value: " \ + "%"PRId64", but should be %"PRId64".\n", \ + #name, (int64_t)current->name, (int64_t)(value)); \ + } \ + } while (0) + #include "cbs_mpeg2_syntax_template.c" #undef READ @@ -105,6 +119,7 @@ #undef xui #undef marker_bit #undef nextbits +#undef infer static void cbs_mpeg2_free_user_data(void *unit, uint8_t *content) diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c index b9d53682fe..87db0ad039 100644 --- a/libavcodec/cbs_mpeg2_syntax_template.c +++ b/libavcodec/cbs_mpeg2_syntax_template.c @@ -144,6 +144,10 @@ static int FUNC(sequence_display_extension)(CodedBitstreamContext *ctx, RWContex uir(8, transfer_characteristics); uir(8, matrix_coefficients); #endif + } else { + infer(colour_primaries, 2); + infer(transfer_characteristics, 2); + infer(matrix_coefficients, 2); } ui(14, display_horizontal_size); diff --git a/libavcodec/mpeg2_metadata_bsf.c b/libavcodec/mpeg2_metadata_bsf.c index ba3a74afda..5aed41a008 100644 --- a/libavcodec/mpeg2_metadata_bsf.c +++ b/libavcodec/mpeg2_metadata_bsf.c @@ -147,18 +147,12 @@ static int mpeg2_metadata_update_fragment(AVBSFContext *bsf, if (ctx->colour_primaries >= 0) sde->colour_primaries = ctx->colour_primaries; - else if (add_sde) - sde->colour_primaries = 2; if (ctx->transfer_characteristics >= 0) sde->transfer_characteristics = ctx->transfer_characteristics; - else if (add_sde) - sde->transfer_characteristics = 2; if (ctx->matrix_coefficients >= 0) sde->matrix_coefficients = ctx->matrix_coefficients; - else if (add_sde) - sde->matrix_coefficients = 2; } } @@ -229,6 +223,18 @@ static int mpeg2_metadata_init(AVBSFContext *bsf) CodedBitstreamFragment *frag = &ctx->fragment; int err; +#define VALIDITY_CHECK(name) do { \ + if (!ctx->name) { \ + av_log(bsf, AV_LOG_ERROR, "The value 0 for %s is " \ + "forbidden.\n", #name); \ + return AVERROR(EINVAL); \ + } \ + } while (0) + VALIDITY_CHECK(colour_primaries); + VALIDITY_CHECK(transfer_characteristics); + VALIDITY_CHECK(matrix_coefficients); +#undef VALIDITY_CHECK + err = ff_cbs_init(&ctx->cbc, AV_CODEC_ID_MPEG2VIDEO, bsf); if (err < 0) return err;
If a sequence display extension is read with colour_description equal to zero, but a user wants to add one or more of the colour_description elements, then the colour_description elements the user did not explicitly request to be set are set to zero and not to the value equal to unknown/unspecified (namely 2). A value of zero is not only inappropriate, but explicitly forbidden. This is fixed by inferring the right default values during the reading process if the elements are absent; moreover, changing any of the colour_description elements to zero is now no longer permitted. Furthermore, if a sequence display extension has to be added, the earlier code set some fields to their default value twice. This has been changed, too. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavcodec/cbs_mpeg2.c | 15 +++++++++++++++ libavcodec/cbs_mpeg2_syntax_template.c | 4 ++++ libavcodec/mpeg2_metadata_bsf.c | 18 ++++++++++++------ 3 files changed, 31 insertions(+), 6 deletions(-)