| Message ID | 20170123233815.14644-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 25f4f08ba5b4d928f8b02ca388e1aa8d37d8e24e |
| Headers | show |
On Tue, 24 Jan 2017 00:38:15 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > This treats the case of no slices like no frames which it basically is. > > The field is added to the context as other nal related fields are also there > and passing the has_slices field per *arguments is ugly and not consistent > > Found-by: ubitux > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/h264dec.c | 6 ++++-- > libavcodec/h264dec.h | 2 ++ > tests/fate/h264.mak | 2 +- > 3 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c > index 4ecaec267c..41e6ce458c 100644 > --- a/libavcodec/h264dec.c > +++ b/libavcodec/h264dec.c > @@ -607,6 +607,7 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size) > int idr_cleared=0; > int i, ret = 0; > > + h->has_slice = 0; > h->nal_unit_type= 0; > > h->max_contexts = h->nb_slice_ctx; > @@ -672,6 +673,7 @@ again: > h->has_recovery_point = 1; > case H264_NAL_SLICE: > sl->gb = nal->gb; > + h->has_slice = 1; > > if ((err = ff_h264_decode_slice_header(h, sl, nal))) > break; > @@ -839,7 +841,7 @@ end: > } > #endif /* CONFIG_ERROR_RESILIENCE */ > /* clean up */ > - if (h->cur_pic_ptr && !h->droppable) { > + if (h->cur_pic_ptr && !h->droppable && h->has_slice) { > ff_thread_report_progress(&h->cur_pic_ptr->tf, INT_MAX, > h->picture_structure == PICT_BOTTOM_FIELD); > } > @@ -1055,7 +1057,7 @@ static int h264_decode_frame(AVCodecContext *avctx, void *data, > return send_next_delayed_frame(h, pict, got_frame, buf_index); > } > > - if (!(avctx->flags2 & AV_CODEC_FLAG2_CHUNKS) && !h->cur_pic_ptr) { > + if (!(avctx->flags2 & AV_CODEC_FLAG2_CHUNKS) && (!h->cur_pic_ptr || !h->has_slice)) { > if (avctx->skip_frame >= AVDISCARD_NONREF || > buf_size >= 4 && !memcmp("Q264", buf, 4)) > return buf_size; > diff --git a/libavcodec/h264dec.h b/libavcodec/h264dec.h > index c8b7e663b3..fa5c98ee90 100644 > --- a/libavcodec/h264dec.h > +++ b/libavcodec/h264dec.h > @@ -446,6 +446,8 @@ typedef struct H264Context { > int nal_ref_idc; > int nal_unit_type; > > + int has_slice; ///< slice NAL is found in the packet, set by decode_nal_units, its state does not need to be preserved outside h264_decode_frame() > + > /** > * Used to parse AVC variant of H.264 > */ > diff --git a/tests/fate/h264.mak b/tests/fate/h264.mak > index d40681f9c9..1f6e5f3947 100644 > --- a/tests/fate/h264.mak > +++ b/tests/fate/h264.mak > @@ -424,7 +424,7 @@ fate-h264-extreme-plane-pred: CMD = framemd5 -i $(TARGET_SAM > fate-h264-interlace-crop: CMD = framecrc -i $(TARGET_SAMPLES)/h264/interlaced_crop.mp4 -vframes 3 > fate-h264-brokensps-2580: CMD = framecrc -i $(TARGET_SAMPLES)/h264/brokensps.flv -vf format=yuv420p,scale=w=192:h=144 -sws_flags bitexact+bilinear > fate-h264-xavc-4389: CMD = framecrc -i $(TARGET_SAMPLES)/h264/SonyXAVC_LongGOP_green_pixelation_early_Frames.MXF -pix_fmt yuv422p10le > -fate-h264-attachment-631: CMD = framecrc -i $(TARGET_SAMPLES)/h264/attachment631-small.mp4 -an -max_error_rate 0.95 > +fate-h264-attachment-631: CMD = framecrc -i $(TARGET_SAMPLES)/h264/attachment631-small.mp4 -an -max_error_rate 0.96 Any reason for this part of the change? It looks to me like the patch is only fixing some regressed logic. > fate-h264-skip-nokey: CMD = framecrc -skip_frame nokey -i $(TARGET_SAMPLES)/h264/h264_intra_first-small.ts > fate-h264-skip-nointra: CMD = framecrc -skip_frame nointra -i $(TARGET_SAMPLES)/h264/h264_intra_first-small.ts > fate-h264-intra-refresh-recovery: CMD = framecrc -i $(TARGET_SAMPLES)/h264/intra_refresh.h264 -frames:v 10
On Tue, Jan 24, 2017 at 07:32:02AM +0100, wm4 wrote: > On Tue, 24 Jan 2017 00:38:15 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > This treats the case of no slices like no frames which it basically is. > > > > The field is added to the context as other nal related fields are also there > > and passing the has_slices field per *arguments is ugly and not consistent > > > > Found-by: ubitux > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/h264dec.c | 6 ++++-- > > libavcodec/h264dec.h | 2 ++ > > tests/fate/h264.mak | 2 +- > > 3 files changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c > > index 4ecaec267c..41e6ce458c 100644 > > --- a/libavcodec/h264dec.c > > +++ b/libavcodec/h264dec.c > > @@ -607,6 +607,7 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size) > > int idr_cleared=0; > > int i, ret = 0; > > > > + h->has_slice = 0; > > h->nal_unit_type= 0; > > > > h->max_contexts = h->nb_slice_ctx; > > @@ -672,6 +673,7 @@ again: > > h->has_recovery_point = 1; > > case H264_NAL_SLICE: > > sl->gb = nal->gb; > > + h->has_slice = 1; > > > > if ((err = ff_h264_decode_slice_header(h, sl, nal))) > > break; > > @@ -839,7 +841,7 @@ end: > > } > > #endif /* CONFIG_ERROR_RESILIENCE */ > > /* clean up */ > > - if (h->cur_pic_ptr && !h->droppable) { > > + if (h->cur_pic_ptr && !h->droppable && h->has_slice) { > > ff_thread_report_progress(&h->cur_pic_ptr->tf, INT_MAX, > > h->picture_structure == PICT_BOTTOM_FIELD); > > } > > @@ -1055,7 +1057,7 @@ static int h264_decode_frame(AVCodecContext *avctx, void *data, > > return send_next_delayed_frame(h, pict, got_frame, buf_index); > > } > > > > - if (!(avctx->flags2 & AV_CODEC_FLAG2_CHUNKS) && !h->cur_pic_ptr) { > > + if (!(avctx->flags2 & AV_CODEC_FLAG2_CHUNKS) && (!h->cur_pic_ptr || !h->has_slice)) { > > if (avctx->skip_frame >= AVDISCARD_NONREF || > > buf_size >= 4 && !memcmp("Q264", buf, 4)) > > return buf_size; > > diff --git a/libavcodec/h264dec.h b/libavcodec/h264dec.h > > index c8b7e663b3..fa5c98ee90 100644 > > --- a/libavcodec/h264dec.h > > +++ b/libavcodec/h264dec.h > > @@ -446,6 +446,8 @@ typedef struct H264Context { > > int nal_ref_idc; > > int nal_unit_type; > > > > + int has_slice; ///< slice NAL is found in the packet, set by decode_nal_units, its state does not need to be preserved outside h264_decode_frame() > > + > > /** > > * Used to parse AVC variant of H.264 > > */ > > diff --git a/tests/fate/h264.mak b/tests/fate/h264.mak > > index d40681f9c9..1f6e5f3947 100644 > > --- a/tests/fate/h264.mak > > +++ b/tests/fate/h264.mak > > @@ -424,7 +424,7 @@ fate-h264-extreme-plane-pred: CMD = framemd5 -i $(TARGET_SAM > > fate-h264-interlace-crop: CMD = framecrc -i $(TARGET_SAMPLES)/h264/interlaced_crop.mp4 -vframes 3 > > fate-h264-brokensps-2580: CMD = framecrc -i $(TARGET_SAMPLES)/h264/brokensps.flv -vf format=yuv420p,scale=w=192:h=144 -sws_flags bitexact+bilinear > > fate-h264-xavc-4389: CMD = framecrc -i $(TARGET_SAMPLES)/h264/SonyXAVC_LongGOP_green_pixelation_early_Frames.MXF -pix_fmt yuv422p10le > > -fate-h264-attachment-631: CMD = framecrc -i $(TARGET_SAMPLES)/h264/attachment631-small.mp4 -an -max_error_rate 0.95 > > +fate-h264-attachment-631: CMD = framecrc -i $(TARGET_SAMPLES)/h264/attachment631-small.mp4 -an -max_error_rate 0.96 > > Any reason for this part of the change? It looks to me like the patch > is only fixing some regressed logic. by treating that case like "no frame" there are more errors returned and this needs an update or the fate test will fail ill apply this as ubitux wants it in for merge work: <ubitux> michaelni: could you push that one btw? [...]
diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c index 4ecaec267c..41e6ce458c 100644 --- a/libavcodec/h264dec.c +++ b/libavcodec/h264dec.c @@ -607,6 +607,7 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size) int idr_cleared=0; int i, ret = 0; + h->has_slice = 0; h->nal_unit_type= 0; h->max_contexts = h->nb_slice_ctx; @@ -672,6 +673,7 @@ again: h->has_recovery_point = 1; case H264_NAL_SLICE: sl->gb = nal->gb; + h->has_slice = 1; if ((err = ff_h264_decode_slice_header(h, sl, nal))) break; @@ -839,7 +841,7 @@ end: } #endif /* CONFIG_ERROR_RESILIENCE */ /* clean up */ - if (h->cur_pic_ptr && !h->droppable) { + if (h->cur_pic_ptr && !h->droppable && h->has_slice) { ff_thread_report_progress(&h->cur_pic_ptr->tf, INT_MAX, h->picture_structure == PICT_BOTTOM_FIELD); } @@ -1055,7 +1057,7 @@ static int h264_decode_frame(AVCodecContext *avctx, void *data, return send_next_delayed_frame(h, pict, got_frame, buf_index); } - if (!(avctx->flags2 & AV_CODEC_FLAG2_CHUNKS) && !h->cur_pic_ptr) { + if (!(avctx->flags2 & AV_CODEC_FLAG2_CHUNKS) && (!h->cur_pic_ptr || !h->has_slice)) { if (avctx->skip_frame >= AVDISCARD_NONREF || buf_size >= 4 && !memcmp("Q264", buf, 4)) return buf_size; diff --git a/libavcodec/h264dec.h b/libavcodec/h264dec.h index c8b7e663b3..fa5c98ee90 100644 --- a/libavcodec/h264dec.h +++ b/libavcodec/h264dec.h @@ -446,6 +446,8 @@ typedef struct H264Context { int nal_ref_idc; int nal_unit_type; + int has_slice; ///< slice NAL is found in the packet, set by decode_nal_units, its state does not need to be preserved outside h264_decode_frame() + /** * Used to parse AVC variant of H.264 */ diff --git a/tests/fate/h264.mak b/tests/fate/h264.mak index d40681f9c9..1f6e5f3947 100644 --- a/tests/fate/h264.mak +++ b/tests/fate/h264.mak @@ -424,7 +424,7 @@ fate-h264-extreme-plane-pred: CMD = framemd5 -i $(TARGET_SAM fate-h264-interlace-crop: CMD = framecrc -i $(TARGET_SAMPLES)/h264/interlaced_crop.mp4 -vframes 3 fate-h264-brokensps-2580: CMD = framecrc -i $(TARGET_SAMPLES)/h264/brokensps.flv -vf format=yuv420p,scale=w=192:h=144 -sws_flags bitexact+bilinear fate-h264-xavc-4389: CMD = framecrc -i $(TARGET_SAMPLES)/h264/SonyXAVC_LongGOP_green_pixelation_early_Frames.MXF -pix_fmt yuv422p10le -fate-h264-attachment-631: CMD = framecrc -i $(TARGET_SAMPLES)/h264/attachment631-small.mp4 -an -max_error_rate 0.95 +fate-h264-attachment-631: CMD = framecrc -i $(TARGET_SAMPLES)/h264/attachment631-small.mp4 -an -max_error_rate 0.96 fate-h264-skip-nokey: CMD = framecrc -skip_frame nokey -i $(TARGET_SAMPLES)/h264/h264_intra_first-small.ts fate-h264-skip-nointra: CMD = framecrc -skip_frame nointra -i $(TARGET_SAMPLES)/h264/h264_intra_first-small.ts fate-h264-intra-refresh-recovery: CMD = framecrc -i $(TARGET_SAMPLES)/h264/intra_refresh.h264 -frames:v 10
This treats the case of no slices like no frames which it basically is. The field is added to the context as other nal related fields are also there and passing the has_slices field per *arguments is ugly and not consistent Found-by: ubitux Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/h264dec.c | 6 ++++-- libavcodec/h264dec.h | 2 ++ tests/fate/h264.mak | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-)