[FFmpeg-devel,2/3] avcodec/h264: error out on ff_h264_pred_weight_table() only if strict spec compliance is requested

Submitted by James Almer on April 9, 2019, 6:32 p.m.

Details

Message ID 20190409183227.7680-2-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer April 9, 2019, 6:32 p.m.
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(-)

Comments

Michael Niedermayer April 10, 2019, 8:24 a.m.
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

[...]
James Almer April 10, 2019, 6:01 p.m.
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".
>
Carl Eugen Hoyos April 10, 2019, 7:47 p.m.
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
Carl Eugen Hoyos April 13, 2019, 8:44 a.m.
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
James Almer April 13, 2019, 1:28 p.m.
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".
>
James Almer April 13, 2019, 1:43 p.m.
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.
Carl Eugen Hoyos April 13, 2019, 1:46 p.m.
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
James Almer April 13, 2019, 1:52 p.m.
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.

Patch hide | download patch | download mbox

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;
     }