| Message ID | 20170403014545.3704-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Sun, Apr 02, 2017 at 10:45:41PM -0300, James Almer wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/hevc_parse.c | 32 +++++++++++++++++++++++++------- > 1 file changed, 25 insertions(+), 7 deletions(-) > > diff --git a/libavcodec/hevc_parse.c b/libavcodec/hevc_parse.c > index 6c1138e015..028ca5afe5 100644 > --- a/libavcodec/hevc_parse.c > +++ b/libavcodec/hevc_parse.c > @@ -22,7 +22,8 @@ > #include "hevc_parse.h" > > static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets *ps, > - int is_nalff, int nal_length_size, void *logctx) > + int is_nalff, int nal_length_size, int err_recognition, > + void *logctx) > { > int i; > int ret = 0; > @@ -38,9 +39,21 @@ static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets > > /* ignore everything except parameter sets and VCL NALUs */ > switch (nal->type) { > - case HEVC_NAL_VPS: ff_hevc_decode_nal_vps(&nal->gb, logctx, ps); break; > - case HEVC_NAL_SPS: ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); break; > - case HEVC_NAL_PPS: ff_hevc_decode_nal_pps(&nal->gb, logctx, ps); break; > + case HEVC_NAL_VPS: > + ret = ff_hevc_decode_nal_vps(&nal->gb, logctx, ps); > + if (ret < 0) > + goto done; > + break; > + case HEVC_NAL_SPS: > + ret = ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); > + if (ret < 0) > + goto done; > + break; > + case HEVC_NAL_PPS: > + ret = ff_hevc_decode_nal_pps(&nal->gb, logctx, ps); > + if (ret < 0) > + goto done; > + break; > case HEVC_NAL_TRAIL_R: > case HEVC_NAL_TRAIL_N: > case HEVC_NAL_TSA_N: I didnt investigate how exactly this is used but from just the patch this seems not optimal For example, if you have 3 PPS NALs and the first fails to decode you might still be able to fully decode the other 2 [...]
On 4/3/2017 7:00 AM, Michael Niedermayer wrote: > On Sun, Apr 02, 2017 at 10:45:41PM -0300, James Almer wrote: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/hevc_parse.c | 32 +++++++++++++++++++++++++------- >> 1 file changed, 25 insertions(+), 7 deletions(-) >> >> diff --git a/libavcodec/hevc_parse.c b/libavcodec/hevc_parse.c >> index 6c1138e015..028ca5afe5 100644 >> --- a/libavcodec/hevc_parse.c >> +++ b/libavcodec/hevc_parse.c >> @@ -22,7 +22,8 @@ >> #include "hevc_parse.h" >> >> static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets *ps, >> - int is_nalff, int nal_length_size, void *logctx) >> + int is_nalff, int nal_length_size, int err_recognition, >> + void *logctx) >> { >> int i; >> int ret = 0; >> @@ -38,9 +39,21 @@ static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets >> >> /* ignore everything except parameter sets and VCL NALUs */ >> switch (nal->type) { >> - case HEVC_NAL_VPS: ff_hevc_decode_nal_vps(&nal->gb, logctx, ps); break; >> - case HEVC_NAL_SPS: ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); break; >> - case HEVC_NAL_PPS: ff_hevc_decode_nal_pps(&nal->gb, logctx, ps); break; >> + case HEVC_NAL_VPS: >> + ret = ff_hevc_decode_nal_vps(&nal->gb, logctx, ps); >> + if (ret < 0) >> + goto done; >> + break; >> + case HEVC_NAL_SPS: >> + ret = ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); >> + if (ret < 0) >> + goto done; >> + break; >> + case HEVC_NAL_PPS: >> + ret = ff_hevc_decode_nal_pps(&nal->gb, logctx, ps); >> + if (ret < 0) >> + goto done; >> + break; >> case HEVC_NAL_TRAIL_R: >> case HEVC_NAL_TRAIL_N: >> case HEVC_NAL_TSA_N: > > I didnt investigate how exactly this is used but from just the patch > this seems not optimal > For example, if you have 3 PPS NALs and the first fails to decode > you might still be able to fully decode the other 2 I'm mimicking the behavior of decode_nal_unit() in hevcdec.c, which is currently used during frame decoding and extradata decoding. This patchset aims at removing duplicate code while keeping the hevc decoder behaving the same as it was before. I could skip this change if that's preferred, but if this behavior is not optimal then someone who better understands the codec should look at it.
On 4/3/2017 10:46 AM, James Almer wrote: > On 4/3/2017 7:00 AM, Michael Niedermayer wrote: >> On Sun, Apr 02, 2017 at 10:45:41PM -0300, James Almer wrote: >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavcodec/hevc_parse.c | 32 +++++++++++++++++++++++++------- >>> 1 file changed, 25 insertions(+), 7 deletions(-) >>> >>> diff --git a/libavcodec/hevc_parse.c b/libavcodec/hevc_parse.c >>> index 6c1138e015..028ca5afe5 100644 >>> --- a/libavcodec/hevc_parse.c >>> +++ b/libavcodec/hevc_parse.c >>> @@ -22,7 +22,8 @@ >>> #include "hevc_parse.h" >>> >>> static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets *ps, >>> - int is_nalff, int nal_length_size, void *logctx) >>> + int is_nalff, int nal_length_size, int err_recognition, >>> + void *logctx) >>> { >>> int i; >>> int ret = 0; >>> @@ -38,9 +39,21 @@ static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets >>> >>> /* ignore everything except parameter sets and VCL NALUs */ >>> switch (nal->type) { >>> - case HEVC_NAL_VPS: ff_hevc_decode_nal_vps(&nal->gb, logctx, ps); break; >>> - case HEVC_NAL_SPS: ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); break; >>> - case HEVC_NAL_PPS: ff_hevc_decode_nal_pps(&nal->gb, logctx, ps); break; >>> + case HEVC_NAL_VPS: >>> + ret = ff_hevc_decode_nal_vps(&nal->gb, logctx, ps); >>> + if (ret < 0) >>> + goto done; >>> + break; >>> + case HEVC_NAL_SPS: >>> + ret = ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); >>> + if (ret < 0) >>> + goto done; >>> + break; >>> + case HEVC_NAL_PPS: >>> + ret = ff_hevc_decode_nal_pps(&nal->gb, logctx, ps); >>> + if (ret < 0) >>> + goto done; >>> + break; >>> case HEVC_NAL_TRAIL_R: >>> case HEVC_NAL_TRAIL_N: >>> case HEVC_NAL_TSA_N: >> >> I didnt investigate how exactly this is used but from just the patch >> this seems not optimal >> For example, if you have 3 PPS NALs and the first fails to decode >> you might still be able to fully decode the other 2 > > I'm mimicking the behavior of decode_nal_unit() in hevcdec.c, which is > currently used during frame decoding and extradata decoding. > This patchset aims at removing duplicate code while keeping the hevc > decoder behaving the same as it was before. I could skip this change > if that's preferred, but if this behavior is not optimal then someone > who better understands the codec should look at it. To add some context, the functions in hevc_parse.c are currently used only by the mediacodec based hevc decoder to decode extradata, and it's a duplicate of existing functionality in hevcdec.c used by the internal hevc decoder. This set aims at putting the hevc_parse.c version on par with the hevcdec.c one (in the scope of extradata parsing, not frame) to share it between the two decoders and any other that may need it in the future. This patch checks for PS failures and aborts if err_recog is set to explode. The second makes apply_defdispwin user defined instead of having it hardcoded to 1. The third avoids aborting on NALs not needed or expected in extradata, and the last two patches make hevcdec.c use ff_hevc_decode_extradata(). Not aborting on PS NAL failures here would technically mean a change in behavior on the internal hevc decoder once patch five is applied, and I'd rather no do that as part of this set.
On Wed, Apr 5, 2017 at 11:40 PM, James Almer <jamrial@gmail.com> wrote: > On 4/3/2017 10:46 AM, James Almer wrote: >> On 4/3/2017 7:00 AM, Michael Niedermayer wrote: >>> On Sun, Apr 02, 2017 at 10:45:41PM -0300, James Almer wrote: >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavcodec/hevc_parse.c | 32 +++++++++++++++++++++++++------- >>>> 1 file changed, 25 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/libavcodec/hevc_parse.c b/libavcodec/hevc_parse.c >>>> index 6c1138e015..028ca5afe5 100644 >>>> --- a/libavcodec/hevc_parse.c >>>> +++ b/libavcodec/hevc_parse.c >>>> @@ -22,7 +22,8 @@ >>>> #include "hevc_parse.h" >>>> >>>> static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets *ps, >>>> - int is_nalff, int nal_length_size, void *logctx) >>>> + int is_nalff, int nal_length_size, int err_recognition, >>>> + void *logctx) >>>> { >>>> int i; >>>> int ret = 0; >>>> @@ -38,9 +39,21 @@ static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets >>>> >>>> /* ignore everything except parameter sets and VCL NALUs */ >>>> switch (nal->type) { >>>> - case HEVC_NAL_VPS: ff_hevc_decode_nal_vps(&nal->gb, logctx, ps); break; >>>> - case HEVC_NAL_SPS: ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); break; >>>> - case HEVC_NAL_PPS: ff_hevc_decode_nal_pps(&nal->gb, logctx, ps); break; >>>> + case HEVC_NAL_VPS: >>>> + ret = ff_hevc_decode_nal_vps(&nal->gb, logctx, ps); >>>> + if (ret < 0) >>>> + goto done; >>>> + break; >>>> + case HEVC_NAL_SPS: >>>> + ret = ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); >>>> + if (ret < 0) >>>> + goto done; >>>> + break; >>>> + case HEVC_NAL_PPS: >>>> + ret = ff_hevc_decode_nal_pps(&nal->gb, logctx, ps); >>>> + if (ret < 0) >>>> + goto done; >>>> + break; >>>> case HEVC_NAL_TRAIL_R: >>>> case HEVC_NAL_TRAIL_N: >>>> case HEVC_NAL_TSA_N: >>> >>> I didnt investigate how exactly this is used but from just the patch >>> this seems not optimal >>> For example, if you have 3 PPS NALs and the first fails to decode >>> you might still be able to fully decode the other 2 >> >> I'm mimicking the behavior of decode_nal_unit() in hevcdec.c, which is >> currently used during frame decoding and extradata decoding. >> This patchset aims at removing duplicate code while keeping the hevc >> decoder behaving the same as it was before. I could skip this change >> if that's preferred, but if this behavior is not optimal then someone >> who better understands the codec should look at it. > > To add some context, the functions in hevc_parse.c are currently used > only by the mediacodec based hevc decoder to decode extradata, and it's > a duplicate of existing functionality in hevcdec.c used by the internal > hevc decoder. > > This set aims at putting the hevc_parse.c version on par with the > hevcdec.c one (in the scope of extradata parsing, not frame) to share it > between the two decoders and any other that may need it in the future. > This patch checks for PS failures and aborts if err_recog is set to > explode. The second makes apply_defdispwin user defined instead of > having it hardcoded to 1. The third avoids aborting on NALs not needed > or expected in extradata, and the last two patches make hevcdec.c use > ff_hevc_decode_extradata(). > > Not aborting on PS NAL failures here would technically mean a change in > behavior on the internal hevc decoder once patch five is applied, and > I'd rather no do that as part of this set. > Keeping the current behavior for the HEVC software decoder seems ideal. If a change in behavior is wanted afterwards, it should be dealt with in a separate change, and not this refactoring. - Hendrik
On 4/5/2017 6:53 PM, Hendrik Leppkes wrote: > On Wed, Apr 5, 2017 at 11:40 PM, James Almer <jamrial@gmail.com> wrote: >> On 4/3/2017 10:46 AM, James Almer wrote: >>> On 4/3/2017 7:00 AM, Michael Niedermayer wrote: >>>> On Sun, Apr 02, 2017 at 10:45:41PM -0300, James Almer wrote: >>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>> --- >>>>> libavcodec/hevc_parse.c | 32 +++++++++++++++++++++++++------- >>>>> 1 file changed, 25 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/libavcodec/hevc_parse.c b/libavcodec/hevc_parse.c >>>>> index 6c1138e015..028ca5afe5 100644 >>>>> --- a/libavcodec/hevc_parse.c >>>>> +++ b/libavcodec/hevc_parse.c >>>>> @@ -22,7 +22,8 @@ >>>>> #include "hevc_parse.h" >>>>> >>>>> static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets *ps, >>>>> - int is_nalff, int nal_length_size, void *logctx) >>>>> + int is_nalff, int nal_length_size, int err_recognition, >>>>> + void *logctx) >>>>> { >>>>> int i; >>>>> int ret = 0; >>>>> @@ -38,9 +39,21 @@ static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets >>>>> >>>>> /* ignore everything except parameter sets and VCL NALUs */ >>>>> switch (nal->type) { >>>>> - case HEVC_NAL_VPS: ff_hevc_decode_nal_vps(&nal->gb, logctx, ps); break; >>>>> - case HEVC_NAL_SPS: ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); break; >>>>> - case HEVC_NAL_PPS: ff_hevc_decode_nal_pps(&nal->gb, logctx, ps); break; >>>>> + case HEVC_NAL_VPS: >>>>> + ret = ff_hevc_decode_nal_vps(&nal->gb, logctx, ps); >>>>> + if (ret < 0) >>>>> + goto done; >>>>> + break; >>>>> + case HEVC_NAL_SPS: >>>>> + ret = ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); >>>>> + if (ret < 0) >>>>> + goto done; >>>>> + break; >>>>> + case HEVC_NAL_PPS: >>>>> + ret = ff_hevc_decode_nal_pps(&nal->gb, logctx, ps); >>>>> + if (ret < 0) >>>>> + goto done; >>>>> + break; >>>>> case HEVC_NAL_TRAIL_R: >>>>> case HEVC_NAL_TRAIL_N: >>>>> case HEVC_NAL_TSA_N: >>>> >>>> I didnt investigate how exactly this is used but from just the patch >>>> this seems not optimal >>>> For example, if you have 3 PPS NALs and the first fails to decode >>>> you might still be able to fully decode the other 2 >>> >>> I'm mimicking the behavior of decode_nal_unit() in hevcdec.c, which is >>> currently used during frame decoding and extradata decoding. >>> This patchset aims at removing duplicate code while keeping the hevc >>> decoder behaving the same as it was before. I could skip this change >>> if that's preferred, but if this behavior is not optimal then someone >>> who better understands the codec should look at it. >> >> To add some context, the functions in hevc_parse.c are currently used >> only by the mediacodec based hevc decoder to decode extradata, and it's >> a duplicate of existing functionality in hevcdec.c used by the internal >> hevc decoder. >> >> This set aims at putting the hevc_parse.c version on par with the >> hevcdec.c one (in the scope of extradata parsing, not frame) to share it >> between the two decoders and any other that may need it in the future. >> This patch checks for PS failures and aborts if err_recog is set to >> explode. The second makes apply_defdispwin user defined instead of >> having it hardcoded to 1. The third avoids aborting on NALs not needed >> or expected in extradata, and the last two patches make hevcdec.c use >> ff_hevc_decode_extradata(). >> >> Not aborting on PS NAL failures here would technically mean a change in >> behavior on the internal hevc decoder once patch five is applied, and >> I'd rather no do that as part of this set. >> > > Keeping the current behavior for the HEVC software decoder seems > ideal. If a change in behavior is wanted afterwards, it should be > dealt with in a separate change, and not this refactoring. > > - Hendrik Patchset pushed then. Thanks.
diff --git a/libavcodec/hevc_parse.c b/libavcodec/hevc_parse.c index 6c1138e015..028ca5afe5 100644 --- a/libavcodec/hevc_parse.c +++ b/libavcodec/hevc_parse.c @@ -22,7 +22,8 @@ #include "hevc_parse.h" static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets *ps, - int is_nalff, int nal_length_size, void *logctx) + int is_nalff, int nal_length_size, int err_recognition, + void *logctx) { int i; int ret = 0; @@ -38,9 +39,21 @@ static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets /* ignore everything except parameter sets and VCL NALUs */ switch (nal->type) { - case HEVC_NAL_VPS: ff_hevc_decode_nal_vps(&nal->gb, logctx, ps); break; - case HEVC_NAL_SPS: ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); break; - case HEVC_NAL_PPS: ff_hevc_decode_nal_pps(&nal->gb, logctx, ps); break; + case HEVC_NAL_VPS: + ret = ff_hevc_decode_nal_vps(&nal->gb, logctx, ps); + if (ret < 0) + goto done; + break; + case HEVC_NAL_SPS: + ret = ff_hevc_decode_nal_sps(&nal->gb, logctx, ps, 1); + if (ret < 0) + goto done; + break; + case HEVC_NAL_PPS: + ret = ff_hevc_decode_nal_pps(&nal->gb, logctx, ps); + if (ret < 0) + goto done; + break; case HEVC_NAL_TRAIL_R: case HEVC_NAL_TRAIL_N: case HEVC_NAL_TSA_N: @@ -66,7 +79,10 @@ static int hevc_decode_nal_units(const uint8_t *buf, int buf_size, HEVCParamSets done: ff_h2645_packet_uninit(&pkt); - return ret; + if (err_recognition & AV_EF_EXPLODE) + return ret; + + return 0; } int ff_hevc_decode_extradata(const uint8_t *data, int size, HEVCParamSets *ps, @@ -109,7 +125,8 @@ int ff_hevc_decode_extradata(const uint8_t *data, int size, HEVCParamSets *ps, return AVERROR_INVALIDDATA; } - ret = hevc_decode_nal_units(gb.buffer, nalsize, ps, *is_nalff, *nal_length_size, logctx); + ret = hevc_decode_nal_units(gb.buffer, nalsize, ps, *is_nalff, *nal_length_size, + err_recognition, logctx); if (ret < 0) { av_log(logctx, AV_LOG_ERROR, "Decoding nal unit %d %d from hvcC failed\n", @@ -125,7 +142,8 @@ int ff_hevc_decode_extradata(const uint8_t *data, int size, HEVCParamSets *ps, *nal_length_size = nal_len_size; } else { *is_nalff = 0; - ret = hevc_decode_nal_units(data, size, ps, *is_nalff, *nal_length_size, logctx); + ret = hevc_decode_nal_units(data, size, ps, *is_nalff, *nal_length_size, + err_recognition, logctx); if (ret < 0) return ret; }
Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/hevc_parse.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-)