| Message ID | 1490621972-38782-1-git-send-email-rsbultje@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, 27 Mar 2017 09:39:32 -0400 "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > The patch introduces race conditions. > --- > libavcodec/h264_slice.c | 3 --- > libavcodec/h264dec.c | 20 -------------------- > libavcodec/h264dec.h | 8 -------- > 3 files changed, 31 deletions(-) > > diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c > index a703853..fa1e9ae 100644 > --- a/libavcodec/h264_slice.c > +++ b/libavcodec/h264_slice.c > @@ -383,9 +383,6 @@ int ff_h264_update_thread_context(AVCodecContext *dst, > h->picture_structure = h1->picture_structure; > h->mb_aff_frame = h1->mb_aff_frame; > h->droppable = h1->droppable; > - h->backup_width = h1->backup_width; > - h->backup_height = h1->backup_height; > - h->backup_pix_fmt = h1->backup_pix_fmt; > > for (i = 0; i < H264_MAX_PICTURE_COUNT; i++) { > ff_h264_unref_picture(h, &h->DPB[i]); > diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c > index 9042169..be69e55 100644 > --- a/libavcodec/h264dec.c > +++ b/libavcodec/h264dec.c > @@ -307,9 +307,6 @@ static int h264_init_context(AVCodecContext *avctx, H264Context *h) > int i; > > h->avctx = avctx; > - h->backup_width = -1; > - h->backup_height = -1; > - h->backup_pix_fmt = AV_PIX_FMT_NONE; > h->cur_chroma_format_idc = -1; > > h->picture_structure = PICT_FRAME; > @@ -861,10 +858,6 @@ static int output_frame(H264Context *h, AVFrame *dst, H264Picture *srcp) > > av_dict_set(&dst->metadata, "stereo_mode", ff_h264_sei_stereo_mode(&h->sei.frame_packing), 0); > > - h->backup_width = h->avctx->width; > - h->backup_height = h->avctx->height; > - h->backup_pix_fmt = h->avctx->pix_fmt; > - > h->avctx->width = dst->width; > h->avctx->height = dst->height; > h->avctx->pix_fmt = dst->format; > @@ -1003,19 +996,6 @@ static int h264_decode_frame(AVCodecContext *avctx, void *data, > h->setup_finished = 0; > h->nb_slice_ctx_queued = 0; > > - if (h->backup_width != -1) { > - avctx->width = h->backup_width; > - h->backup_width = -1; > - } > - if (h->backup_height != -1) { > - avctx->height = h->backup_height; > - h->backup_height = -1; > - } > - if (h->backup_pix_fmt != AV_PIX_FMT_NONE) { > - avctx->pix_fmt = h->backup_pix_fmt; > - h->backup_pix_fmt = AV_PIX_FMT_NONE; > - } > - > ff_h264_unref_picture(h, &h->last_pic_for_ec); > > /* end of stream, output what is still in the buffers */ > diff --git a/libavcodec/h264dec.h b/libavcodec/h264dec.h > index 5f868b7..e994f7e 100644 > --- a/libavcodec/h264dec.h > +++ b/libavcodec/h264dec.h > @@ -363,14 +363,6 @@ typedef struct H264Context { > int width, height; > int chroma_x_shift, chroma_y_shift; > > - /** > - * Backup frame properties: needed, because they can be different > - * between returned frame and last decoded frame. > - **/ > - int backup_width; > - int backup_height; > - enum AVPixelFormat backup_pix_fmt; > - > int droppable; > int coded_picture_number; > Why was this even applied? The commit message says it fixes "Inconsistencies between the dimensions/pixel format of avctx and the frame", but shouldn't that be done strictly in utils.c.
Hi, On Mon, Mar 27, 2017 at 9:49 AM, wm4 <nfxjfg@googlemail.com> wrote: > On Mon, 27 Mar 2017 09:39:32 -0400 > "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > > > The patch introduces race conditions. > > --- > > libavcodec/h264_slice.c | 3 --- > > libavcodec/h264dec.c | 20 -------------------- > > libavcodec/h264dec.h | 8 -------- > > 3 files changed, 31 deletions(-) > > > > diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c > > index a703853..fa1e9ae 100644 > > --- a/libavcodec/h264_slice.c > > +++ b/libavcodec/h264_slice.c > > @@ -383,9 +383,6 @@ int ff_h264_update_thread_context(AVCodecContext > *dst, > > h->picture_structure = h1->picture_structure; > > h->mb_aff_frame = h1->mb_aff_frame; > > h->droppable = h1->droppable; > > - h->backup_width = h1->backup_width; > > - h->backup_height = h1->backup_height; > > - h->backup_pix_fmt = h1->backup_pix_fmt; > > > > for (i = 0; i < H264_MAX_PICTURE_COUNT; i++) { > > ff_h264_unref_picture(h, &h->DPB[i]); > > diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c > > index 9042169..be69e55 100644 > > --- a/libavcodec/h264dec.c > > +++ b/libavcodec/h264dec.c > > @@ -307,9 +307,6 @@ static int h264_init_context(AVCodecContext *avctx, > H264Context *h) > > int i; > > > > h->avctx = avctx; > > - h->backup_width = -1; > > - h->backup_height = -1; > > - h->backup_pix_fmt = AV_PIX_FMT_NONE; > > h->cur_chroma_format_idc = -1; > > > > h->picture_structure = PICT_FRAME; > > @@ -861,10 +858,6 @@ static int output_frame(H264Context *h, AVFrame > *dst, H264Picture *srcp) > > > > av_dict_set(&dst->metadata, "stereo_mode", > ff_h264_sei_stereo_mode(&h->sei.frame_packing), 0); > > > > - h->backup_width = h->avctx->width; > > - h->backup_height = h->avctx->height; > > - h->backup_pix_fmt = h->avctx->pix_fmt; > > - > > h->avctx->width = dst->width; > > h->avctx->height = dst->height; > > h->avctx->pix_fmt = dst->format; > > @@ -1003,19 +996,6 @@ static int h264_decode_frame(AVCodecContext > *avctx, void *data, > > h->setup_finished = 0; > > h->nb_slice_ctx_queued = 0; > > > > - if (h->backup_width != -1) { > > - avctx->width = h->backup_width; > > - h->backup_width = -1; > > - } > > - if (h->backup_height != -1) { > > - avctx->height = h->backup_height; > > - h->backup_height = -1; > > - } > > - if (h->backup_pix_fmt != AV_PIX_FMT_NONE) { > > - avctx->pix_fmt = h->backup_pix_fmt; > > - h->backup_pix_fmt = AV_PIX_FMT_NONE; > > - } > > - > > ff_h264_unref_picture(h, &h->last_pic_for_ec); > > > > /* end of stream, output what is still in the buffers */ > > diff --git a/libavcodec/h264dec.h b/libavcodec/h264dec.h > > index 5f868b7..e994f7e 100644 > > --- a/libavcodec/h264dec.h > > +++ b/libavcodec/h264dec.h > > @@ -363,14 +363,6 @@ typedef struct H264Context { > > int width, height; > > int chroma_x_shift, chroma_y_shift; > > > > - /** > > - * Backup frame properties: needed, because they can be different > > - * between returned frame and last decoded frame. > > - **/ > > - int backup_width; > > - int backup_height; > > - enum AVPixelFormat backup_pix_fmt; > > - > > int droppable; > > int coded_picture_number; > > > > Why was this even applied? > I've started wondering the same thing. Going back to the patch discussion on the list, it looks like two developers (Michael and Hendrik) had objections, but their comments were ignored. I did not see any comments explicitly approving the patch. Ronald
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c index a703853..fa1e9ae 100644 --- a/libavcodec/h264_slice.c +++ b/libavcodec/h264_slice.c @@ -383,9 +383,6 @@ int ff_h264_update_thread_context(AVCodecContext *dst, h->picture_structure = h1->picture_structure; h->mb_aff_frame = h1->mb_aff_frame; h->droppable = h1->droppable; - h->backup_width = h1->backup_width; - h->backup_height = h1->backup_height; - h->backup_pix_fmt = h1->backup_pix_fmt; for (i = 0; i < H264_MAX_PICTURE_COUNT; i++) { ff_h264_unref_picture(h, &h->DPB[i]); diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c index 9042169..be69e55 100644 --- a/libavcodec/h264dec.c +++ b/libavcodec/h264dec.c @@ -307,9 +307,6 @@ static int h264_init_context(AVCodecContext *avctx, H264Context *h) int i; h->avctx = avctx; - h->backup_width = -1; - h->backup_height = -1; - h->backup_pix_fmt = AV_PIX_FMT_NONE; h->cur_chroma_format_idc = -1; h->picture_structure = PICT_FRAME; @@ -861,10 +858,6 @@ static int output_frame(H264Context *h, AVFrame *dst, H264Picture *srcp) av_dict_set(&dst->metadata, "stereo_mode", ff_h264_sei_stereo_mode(&h->sei.frame_packing), 0); - h->backup_width = h->avctx->width; - h->backup_height = h->avctx->height; - h->backup_pix_fmt = h->avctx->pix_fmt; - h->avctx->width = dst->width; h->avctx->height = dst->height; h->avctx->pix_fmt = dst->format; @@ -1003,19 +996,6 @@ static int h264_decode_frame(AVCodecContext *avctx, void *data, h->setup_finished = 0; h->nb_slice_ctx_queued = 0; - if (h->backup_width != -1) { - avctx->width = h->backup_width; - h->backup_width = -1; - } - if (h->backup_height != -1) { - avctx->height = h->backup_height; - h->backup_height = -1; - } - if (h->backup_pix_fmt != AV_PIX_FMT_NONE) { - avctx->pix_fmt = h->backup_pix_fmt; - h->backup_pix_fmt = AV_PIX_FMT_NONE; - } - ff_h264_unref_picture(h, &h->last_pic_for_ec); /* end of stream, output what is still in the buffers */ diff --git a/libavcodec/h264dec.h b/libavcodec/h264dec.h index 5f868b7..e994f7e 100644 --- a/libavcodec/h264dec.h +++ b/libavcodec/h264dec.h @@ -363,14 +363,6 @@ typedef struct H264Context { int width, height; int chroma_x_shift, chroma_y_shift; - /** - * Backup frame properties: needed, because they can be different - * between returned frame and last decoded frame. - **/ - int backup_width; - int backup_height; - enum AVPixelFormat backup_pix_fmt; - int droppable; int coded_picture_number;