| Message ID | 20190409183227.7680-2-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Apr 09, 2019 at 03:32:26PM -0300, James Almer wrote: > Fixes ticket #7174. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > This makes what's essentially a non spec compliant stream decodable again with > no visual artifacts, and without reintroducing the risk of overflows. > > Alternatively, we could clip the luma and chroma values to the -128..127 > range instead of setting them to defaults. I think to awnser this we would need streams where there is an actual difference between these choices There are 2 possible sources of such non compliant values 1. damaged bitstream data 2. buggy encoders For handling 2. its likely best to narrowly detect the bug and handle it accordingly (which may be to ignore it if for example there are no MBs that use the values in the bug case) For handling 1. its likely best to set all values to some default or guessed not just the first and then continue parsing as the decoder state is already screwed and the following values even if they end in valid ranges likely would be no good. The low number of samples for these 2 cases makes testing what is best somewhat difficult. We could generate damaged streams though but still as theres a wide range of potential damage that may not match the distribution of real world damaged streams I would suggest to leave a request for samples in the codepath. As more samples (damaged as well as buggy encoders) would allow better evaluating which alternative solution would be best in practice and not just theory Thanks [...]
On 4/10/2019 5:24 AM, Michael Niedermayer wrote: > On Tue, Apr 09, 2019 at 03:32:26PM -0300, James Almer wrote: >> Fixes ticket #7174. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> This makes what's essentially a non spec compliant stream decodable again with >> no visual artifacts, and without reintroducing the risk of overflows. >> > >> Alternatively, we could clip the luma and chroma values to the -128..127 >> range instead of setting them to defaults. > > I think to awnser this we would need streams where there is an actual > difference between these choices > > There are 2 possible sources of such non compliant values > 1. damaged bitstream data > 2. buggy encoders > > For handling 2. its likely best to narrowly detect the bug and handle it > accordingly (which may be to ignore it if for example there are no MBs that > use the values in the bug case) > > For handling 1. its likely best to set all values to some default or guessed > not just the first and then continue parsing as the decoder state is already > screwed and the following values even if they end in valid ranges likely > would be no good. > > The low number of samples for these 2 cases makes testing what is best > somewhat difficult. We could generate damaged streams though but still > as theres a wide range of potential damage that may not match the distribution > of real world damaged streams > > I would suggest to leave a request for samples in the codepath. As more > samples (damaged as well as buggy encoders) would allow better evaluating > which alternative solution would be best in practice and not just theory At least with the sample in the ticket, it's a buggy encoder that for some reason coded a -129 value in a field with a -128,127 valid range. The rest of the bitstream appears correct, including byte alignment padding bits and such. I'll replace the av_log lines with request sample ones then. > > Thanks > > [...] > > > _______________________________________________ > 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". >
2019-04-09 20:32 GMT+02:00, James Almer <jamrial@gmail.com>: > Fixes ticket #7174. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > This makes what's essentially a non spec compliant > stream decodable again with no visual artifacts, and > without reintroducing the risk of overflows. Your patch leads to bit-exact output with the reference decoder and old FFmpeg. Carl Eugen
2019-04-10 21:47 GMT+02:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: > 2019-04-09 20:32 GMT+02:00, James Almer <jamrial@gmail.com>: >> Fixes ticket #7174. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> This makes what's essentially a non spec compliant >> stream decodable again with no visual artifacts, and >> without reintroducing the risk of overflows. > > Your patch leads to bit-exact output with the reference > decoder and old FFmpeg. This is true for the original sample, the new sample zNqp.h264 has bit-exact output with old FFmpeg but not with your patch. Carl Eugen
On 4/13/2019 5:44 AM, Carl Eugen Hoyos wrote: > 2019-04-10 21:47 GMT+02:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: >> 2019-04-09 20:32 GMT+02:00, James Almer <jamrial@gmail.com>: >>> Fixes ticket #7174. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> This makes what's essentially a non spec compliant >>> stream decodable again with no visual artifacts, and >>> without reintroducing the risk of overflows. >> >> Your patch leads to bit-exact output with the reference >> decoder and old FFmpeg. > > This is true for the original sample, the new sample > zNqp.h264 has bit-exact output with old FFmpeg > but not with your patch. Where can i get this sample? Perhaps clipping the bogus value will make it bitexact. It would be nice to know what the reference decoder does, for that matter. If it's bothering like us to check and handle this out of spec values, or if they do like we used to and just pass them through. > > Carl Eugen > _______________________________________________ > 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 4/13/2019 10:28 AM, James Almer wrote: > On 4/13/2019 5:44 AM, Carl Eugen Hoyos wrote: >> 2019-04-10 21:47 GMT+02:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: >>> 2019-04-09 20:32 GMT+02:00, James Almer <jamrial@gmail.com>: >>>> Fixes ticket #7174. >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> This makes what's essentially a non spec compliant >>>> stream decodable again with no visual artifacts, and >>>> without reintroducing the risk of overflows. >>> >>> Your patch leads to bit-exact output with the reference >>> decoder and old FFmpeg. >> >> This is true for the original sample, the new sample >> zNqp.h264 has bit-exact output with old FFmpeg >> but not with your patch. > > Where can i get this sample? Perhaps clipping the bogus value will make > it bitexact. > > It would be nice to know what the reference decoder does, for that > matter. If it's bothering like us to check and handle this out of spec > values, or if they do like we used to and just pass them through. Found it and tested the above, and with clipping it still doesn't seem to be bitexact with old ffmpeg.
2019-04-13 15:28 GMT+02:00, James Almer <jamrial@gmail.com>:
> It would be nice to know what the reference decoder does
That's why I told you;-)
Carl Eugen
On 4/13/2019 10:46 AM, Carl Eugen Hoyos wrote: > 2019-04-13 15:28 GMT+02:00, James Almer <jamrial@gmail.com>: > >> It would be nice to know what the reference decoder does > > That's why I told you;-) I must have missed it. Outside of saying its output is bitexact for the first sample as ffmpeg after my patch, i don't think you mentioned what's its behavior for these out of spec values.
diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c index 8bdd886000..1b162b7361 100644 --- a/libavcodec/h264_parse.c +++ b/libavcodec/h264_parse.c @@ -31,6 +31,7 @@ int ff_h264_pred_weight_table(GetBitContext *gb, const SPS *sps, { int list, i, j; int luma_def, chroma_def; + int ret = 0; pwt->use_weight = 0; pwt->use_weight_chroma = 0; @@ -39,6 +40,7 @@ int ff_h264_pred_weight_table(GetBitContext *gb, const SPS *sps, if (pwt->luma_log2_weight_denom > 7U) { av_log(logctx, AV_LOG_ERROR, "luma_log2_weight_denom %d is out of range\n", pwt->luma_log2_weight_denom); pwt->luma_log2_weight_denom = 0; + ret = AVERROR_INVALIDDATA; } luma_def = 1 << pwt->luma_log2_weight_denom; @@ -47,6 +49,7 @@ int ff_h264_pred_weight_table(GetBitContext *gb, const SPS *sps, if (pwt->chroma_log2_weight_denom > 7U) { av_log(logctx, AV_LOG_ERROR, "chroma_log2_weight_denom %d is out of range\n", pwt->chroma_log2_weight_denom); pwt->chroma_log2_weight_denom = 0; + ret = AVERROR_INVALIDDATA; } chroma_def = 1 << pwt->chroma_log2_weight_denom; } @@ -63,9 +66,10 @@ int ff_h264_pred_weight_table(GetBitContext *gb, const SPS *sps, pwt->luma_weight[i][list][1] = get_se_golomb(gb); if ((int8_t)pwt->luma_weight[i][list][0] != pwt->luma_weight[i][list][0] || (int8_t)pwt->luma_weight[i][list][1] != pwt->luma_weight[i][list][1]) { + av_log(logctx, AV_LOG_ERROR, "luma weight is out of range\n"); pwt->luma_weight[i][list][0] = luma_def; pwt->luma_weight[i][list][1] = 0; - goto out_range_weight; + ret = AVERROR_INVALIDDATA; } if (pwt->luma_weight[i][list][0] != luma_def || pwt->luma_weight[i][list][1] != 0) { @@ -86,9 +90,10 @@ int ff_h264_pred_weight_table(GetBitContext *gb, const SPS *sps, pwt->chroma_weight[i][list][j][1] = get_se_golomb(gb); if ((int8_t)pwt->chroma_weight[i][list][j][0] != pwt->chroma_weight[i][list][j][0] || (int8_t)pwt->chroma_weight[i][list][j][1] != pwt->chroma_weight[i][list][j][1]) { + av_log(logctx, AV_LOG_ERROR, "chroma weight is out of range\n"); pwt->chroma_weight[i][list][j][0] = chroma_def; pwt->chroma_weight[i][list][j][1] = 0; - goto out_range_weight; + ret = AVERROR_INVALIDDATA; } if (pwt->chroma_weight[i][list][j][0] != chroma_def || pwt->chroma_weight[i][list][j][1] != 0) { @@ -121,10 +126,8 @@ int ff_h264_pred_weight_table(GetBitContext *gb, const SPS *sps, break; } pwt->use_weight = pwt->use_weight || pwt->use_weight_chroma; - return 0; -out_range_weight: - avpriv_request_sample(logctx, "Out of range weight"); - return AVERROR_INVALIDDATA; + + return ret; } /** diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c index 1c9a270fb6..d3432d7b1f 100644 --- a/libavcodec/h264_slice.c +++ b/libavcodec/h264_slice.c @@ -1867,7 +1867,7 @@ static int h264_slice_header_parse(const H264Context *h, H264SliceContext *sl, ret = ff_h264_pred_weight_table(&sl->gb, sps, sl->ref_count, sl->slice_type_nos, &sl->pwt, picture_structure, h->avctx); - if (ret < 0) + if (ret < 0 && (h->avctx->err_recognition & (AV_EF_BITSTREAM|AV_EF_COMPLIANT))) return ret; }
Fixes ticket #7174. Signed-off-by: James Almer <jamrial@gmail.com> --- This makes what's essentially a non spec compliant stream decodable again with no visual artifacts, and without reintroducing the risk of overflows. Alternatively, we could clip the luma and chroma values to the -128..127 range instead of setting them to defaults. libavcodec/h264_parse.c | 15 +++++++++------ libavcodec/h264_slice.c | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-)