| Message ID | 20200611161320.5136-2-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/cbs_h2645: keep separate parameter set lists for reading and writing | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Am Do., 11. Juni 2020 um 18:14 Uhr schrieb James Almer <jamrial@gmail.com>: > > If this happens, it's a sign of parsing issues earlier in the process, or > misuse by the calling module. > > Prevents creating invalid bitstreams. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/cbs_av1.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c > index fc228086c2..456bd9b1d5 100644 > --- a/libavcodec/cbs_av1.c > +++ b/libavcodec/cbs_av1.c > @@ -711,10 +711,11 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc) > > #define infer(name, value) do { \ > if (current->name != (value)) { \ > - av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \ > + av_log(ctx->log_ctx, AV_LOG_ERROR, \ > "%s does not match inferred value: " \ > "%"PRId64", but should be %"PRId64".\n", \ > #name, (int64_t)current->name, (int64_t)(value)); \ > + return AVERROR_BUG; \ For this return value, please explain in the commit message why this can never happen and in the mail why you are not using an assert(). Carl Eugen
On 6/14/2020 11:11 AM, Carl Eugen Hoyos wrote: > Am Do., 11. Juni 2020 um 18:14 Uhr schrieb James Almer <jamrial@gmail.com>: >> >> If this happens, it's a sign of parsing issues earlier in the process, or >> misuse by the calling module. >> >> Prevents creating invalid bitstreams. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/cbs_av1.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c >> index fc228086c2..456bd9b1d5 100644 >> --- a/libavcodec/cbs_av1.c >> +++ b/libavcodec/cbs_av1.c >> @@ -711,10 +711,11 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc) >> >> #define infer(name, value) do { \ >> if (current->name != (value)) { \ >> - av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \ >> + av_log(ctx->log_ctx, AV_LOG_ERROR, \ >> "%s does not match inferred value: " \ >> "%"PRId64", but should be %"PRId64".\n", \ >> #name, (int64_t)current->name, (int64_t)(value)); \ > >> + return AVERROR_BUG; \ > > For this return value, please explain in the commit message > why this can never happen and in the mail why you are not > using an assert(). I could, i suggested it. But nobody commented if they preferred it or not, so i went with a normal error code. I could also just use the invalid data code, same as the other cases here. I'm fine with any of those options.
On Thu, Jun 11, 2020 at 01:13:20PM -0300, James Almer wrote: > If this happens, it's a sign of parsing issues earlier in the process, or > misuse by the calling module. > > Prevents creating invalid bitstreams. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/cbs_av1.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Iam ok with this but there are other people who know this code better than i do. Also long term, it may be interresting to split the check between inferences that refer to security vs. ones refering to compliance There may be situations where passing non compliant values through is desirable thx [...]
On 6/14/2020 1:24 PM, Michael Niedermayer wrote: > On Thu, Jun 11, 2020 at 01:13:20PM -0300, James Almer wrote: >> If this happens, it's a sign of parsing issues earlier in the process, or >> misuse by the calling module. >> >> Prevents creating invalid bitstreams. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/cbs_av1.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) > > Iam ok with this but there are other people who know this code > better than i do. Pushed and backported these two patches, but postponed the split reference one. I want Mark's opinion on the approach before applying it. Thanks. > > Also long term, it may be interresting to split the check between > inferences that refer to security vs. ones refering to compliance > There may be situations where passing non compliant values through > is desirable > > thx > > [...] > > > _______________________________________________ > 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/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c index fc228086c2..456bd9b1d5 100644 --- a/libavcodec/cbs_av1.c +++ b/libavcodec/cbs_av1.c @@ -711,10 +711,11 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc) #define infer(name, value) do { \ if (current->name != (value)) { \ - av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \ + av_log(ctx->log_ctx, AV_LOG_ERROR, \ "%s does not match inferred value: " \ "%"PRId64", but should be %"PRId64".\n", \ #name, (int64_t)current->name, (int64_t)(value)); \ + return AVERROR_BUG; \ } \ } while (0)
If this happens, it's a sign of parsing issues earlier in the process, or misuse by the calling module. Prevents creating invalid bitstreams. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/cbs_av1.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)