Re: [libav-devel] [PATCH 02/28] h264: move the block starting a new field out of slice_header_parse()

[Janne Grunau]

On 2016-06-09 10:29:48 +0200, Anton Khirnov wrote:
> There is no bitstream parsing in that block and messing with
> decoder-global state is not something that belongs into header parsing.
> 
> Nothing else in this function depends on the value of current_slice,
> except for two validity checks. Those checks are also moved out of
> slice_header_parse().
> ---
>  libavcodec/h264_slice.c | 68 
> -
>  1 file changed, 33 insertions(+), 35 deletions(-)

ok

Janne
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 02/28] h264: move the block starting a new field out of slice_header_parse()

[Anton Khirnov]

Quoting Vittorio Giovara (2016-06-09 18:08:23)
> On Thu, Jun 9, 2016 at 4:29 AM, Anton Khirnov  wrote:
> > There is no bitstream parsing in that block and messing with
> > decoder-global state is not something that belongs into header parsing.
> >
> > Nothing else in this function depends on the value of current_slice,
> > except for two validity checks. Those checks are also moved out of
> > slice_header_parse().
> > ---
> >  libavcodec/h264_slice.c | 68 
> > -
> >  1 file changed, 33 insertions(+), 35 deletions(-)
> >
> > diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> > index 17608b4..c43a0be 100644
> > --- a/libavcodec/h264_slice.c
> > +++ b/libavcodec/h264_slice.c
> > @@ -1177,21 +1177,6 @@ static int h264_slice_header_parse(H264Context *h, 
> > H264SliceContext *sl,
> >
> >  sl->first_mb_addr = get_ue_golomb(>gb);
> >
> > -if (sl->first_mb_addr == 0) { // FIXME better field boundary detection
> > -if (h->current_slice && h->cur_pic_ptr && FIELD_PICTURE(h)) {
> > -ff_h264_field_end(h, sl, 1);
> > -}
> > -
> > -h->current_slice = 0;
> > -if (!h->first_field) {
> > -if (h->cur_pic_ptr && !h->droppable) {
> > -ff_thread_report_progress(>cur_pic_ptr->tf, INT_MAX,
> > -  h->picture_structure == 
> > PICT_BOTTOM_FIELD);
> > -}
> > -h->cur_pic_ptr = NULL;
> > -}
> > -}
> > -
> >  slice_type = get_ue_golomb_31(>gb);
> >  if (slice_type > 9) {
> >  av_log(h->avctx, AV_LOG_ERROR,
> > @@ -1226,11 +1211,6 @@ static int h264_slice_header_parse(H264Context *h, 
> > H264SliceContext *sl,
> > sl->pps_id);
> >  return AVERROR_INVALIDDATA;
> >  }
> > -if (h->current_slice > 0 &&
> > -h->ps.pps != (const PPS*)h->ps.pps_list[sl->pps_id]->data) {
> > -av_log(h->avctx, AV_LOG_ERROR, "PPS changed between slices\n");
> > -return AVERROR_INVALIDDATA;
> > -}
> >  pps = (const PPS*)h->ps.pps_list[sl->pps_id]->data;
> >
> >  if (!h->ps.sps_list[pps->sps_id]) {
> > @@ -1265,21 +1245,6 @@ static int h264_slice_header_parse(H264Context *h, 
> > H264SliceContext *sl,
> >  sl->picture_structure  = picture_structure;
> >  sl->mb_field_decoding_flag = picture_structure != PICT_FRAME;
> >
> > -if (h->current_slice != 0) {
> > -if (h->picture_structure != picture_structure ||
> > -h->droppable != droppable) {
> > -av_log(h->avctx, AV_LOG_ERROR,
> > -   "Changing field mode (%d -> %d) between slices is not 
> > allowed\n",
> > -   h->picture_structure, picture_structure);
> > -return AVERROR_INVALIDDATA;
> > -} else if (!h->cur_pic_ptr) {
> > -av_log(h->avctx, AV_LOG_ERROR,
> > -   "unset cur_pic_ptr on slice %d\n",
> > -   h->current_slice + 1);
> > -return AVERROR_INVALIDDATA;
> > -}
> > -}
> > -
> >  if (picture_structure == PICT_FRAME) {
> >  h->curr_pic_num = h->poc.frame_num;
> >  h->max_pic_num  = 1 << sps->log2_max_frame_num;
> > @@ -1430,10 +1395,43 @@ int ff_h264_decode_slice_header(H264Context *h, 
> > H264SliceContext *sl,
> >  if (ret < 0)
> >  return ret;
> >
> > +if (sl->first_mb_addr == 0) { // FIXME better field boundary detection
> 
> does this FIXME still apply?

Yes, this code is still rather hacky, e.g. it will break on slice
threading.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 02/28] h264: move the block starting a new field out of slice_header_parse()

[Vittorio Giovara]

On Thu, Jun 9, 2016 at 4:29 AM, Anton Khirnov  wrote:
> There is no bitstream parsing in that block and messing with
> decoder-global state is not something that belongs into header parsing.
>
> Nothing else in this function depends on the value of current_slice,
> except for two validity checks. Those checks are also moved out of
> slice_header_parse().
> ---
>  libavcodec/h264_slice.c | 68 
> -
>  1 file changed, 33 insertions(+), 35 deletions(-)
>
> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index 17608b4..c43a0be 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -1177,21 +1177,6 @@ static int h264_slice_header_parse(H264Context *h, 
> H264SliceContext *sl,
>
>  sl->first_mb_addr = get_ue_golomb(>gb);
>
> -if (sl->first_mb_addr == 0) { // FIXME better field boundary detection
> -if (h->current_slice && h->cur_pic_ptr && FIELD_PICTURE(h)) {
> -ff_h264_field_end(h, sl, 1);
> -}
> -
> -h->current_slice = 0;
> -if (!h->first_field) {
> -if (h->cur_pic_ptr && !h->droppable) {
> -ff_thread_report_progress(>cur_pic_ptr->tf, INT_MAX,
> -  h->picture_structure == 
> PICT_BOTTOM_FIELD);
> -}
> -h->cur_pic_ptr = NULL;
> -}
> -}
> -
>  slice_type = get_ue_golomb_31(>gb);
>  if (slice_type > 9) {
>  av_log(h->avctx, AV_LOG_ERROR,
> @@ -1226,11 +1211,6 @@ static int h264_slice_header_parse(H264Context *h, 
> H264SliceContext *sl,
> sl->pps_id);
>  return AVERROR_INVALIDDATA;
>  }
> -if (h->current_slice > 0 &&
> -h->ps.pps != (const PPS*)h->ps.pps_list[sl->pps_id]->data) {
> -av_log(h->avctx, AV_LOG_ERROR, "PPS changed between slices\n");
> -return AVERROR_INVALIDDATA;
> -}
>  pps = (const PPS*)h->ps.pps_list[sl->pps_id]->data;
>
>  if (!h->ps.sps_list[pps->sps_id]) {
> @@ -1265,21 +1245,6 @@ static int h264_slice_header_parse(H264Context *h, 
> H264SliceContext *sl,
>  sl->picture_structure  = picture_structure;
>  sl->mb_field_decoding_flag = picture_structure != PICT_FRAME;
>
> -if (h->current_slice != 0) {
> -if (h->picture_structure != picture_structure ||
> -h->droppable != droppable) {
> -av_log(h->avctx, AV_LOG_ERROR,
> -   "Changing field mode (%d -> %d) between slices is not 
> allowed\n",
> -   h->picture_structure, picture_structure);
> -return AVERROR_INVALIDDATA;
> -} else if (!h->cur_pic_ptr) {
> -av_log(h->avctx, AV_LOG_ERROR,
> -   "unset cur_pic_ptr on slice %d\n",
> -   h->current_slice + 1);
> -return AVERROR_INVALIDDATA;
> -}
> -}
> -
>  if (picture_structure == PICT_FRAME) {
>  h->curr_pic_num = h->poc.frame_num;
>  h->max_pic_num  = 1 << sps->log2_max_frame_num;
> @@ -1430,10 +1395,43 @@ int ff_h264_decode_slice_header(H264Context *h, 
> H264SliceContext *sl,
>  if (ret < 0)
>  return ret;
>
> +if (sl->first_mb_addr == 0) { // FIXME better field boundary detection

does this FIXME still apply?

> +if (h->current_slice && h->cur_pic_ptr && FIELD_PICTURE(h)) {
> +ff_h264_field_end(h, sl, 1);
> +}
> +
> +h->current_slice = 0;
> +if (!h->first_field) {
> +if (h->cur_pic_ptr && !h->droppable) {
> +ff_thread_report_progress(>cur_pic_ptr->tf, INT_MAX,
> +  h->picture_structure == 
> PICT_BOTTOM_FIELD);
> +}
> +h->cur_pic_ptr = NULL;
> +}
> +}
> +
>  if (h->current_slice == 0) {
>  ret = h264_field_start(h, sl, nal);
>  if (ret < 0)
>  return ret;
> +} else {
> +if (h->ps.pps != (const PPS*)h->ps.pps_list[sl->pps_id]->data) {
> +av_log(h->avctx, AV_LOG_ERROR, "PPS changed between slices\n");
> +return AVERROR_INVALIDDATA;
> +}
> +
> +if (h->picture_structure != sl->picture_structure ||
> +h->droppable != (nal->ref_idc == 0)) {
> +av_log(h->avctx, AV_LOG_ERROR,
> +   "Changing field mode (%d -> %d) between slices is not 
> allowed\n",
> +   h->picture_structure, sl->picture_structure);
> +return AVERROR_INVALIDDATA;
> +} else if (!h->cur_pic_ptr) {
> +av_log(h->avctx, AV_LOG_ERROR,
> +   "unset cur_pic_ptr on slice %d\n",
> +   h->current_slice + 1);
> +return AVERROR_INVALIDDATA;
> +}
>  }
>
>  assert(h->mb_num == h->mb_width * h->mb_height);
> --

probably ok
-- 
Vittorio
___
libav-devel mailing list