diff mbox series

[FFmpeg-devel] hevc_parser: drop the use of SliceHeader

Message ID 20200527090343.10646-1-anton@khirnov.net
State Accepted
Headers show
Series [FFmpeg-devel] hevc_parser: drop the use of SliceHeader
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Anton Khirnov May 27, 2020, 9:03 a.m. UTC
It is only used to store a few local variables within one function,
which is better accomplished by just declaring them on stack explicitly.

Move SliceHeader back from hevc_ps.h to hevdec.h, since it is not
related to parameters sets.
---
 libavcodec/hevc_parser.c | 57 +++++++++++++++--------------
 libavcodec/hevc_ps.h     | 77 ----------------------------------------
 libavcodec/hevcdec.h     | 77 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+), 104 deletions(-)

Comments

James Almer May 27, 2020, 12:41 p.m. UTC | #1
On 5/27/2020 6:03 AM, Anton Khirnov wrote:
> It is only used to store a few local variables within one function,
> which is better accomplished by just declaring them on stack explicitly.
> 
> Move SliceHeader back from hevc_ps.h to hevdec.h, since it is not
> related to parameters sets.
> ---
>  libavcodec/hevc_parser.c | 57 +++++++++++++++--------------
>  libavcodec/hevc_ps.h     | 77 ----------------------------------------
>  libavcodec/hevcdec.h     | 77 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 107 insertions(+), 104 deletions(-)
> 
> diff --git a/libavcodec/hevc_parser.c b/libavcodec/hevc_parser.c
> index 84f19b485c..5af4b788d5 100644
> --- a/libavcodec/hevc_parser.c
> +++ b/libavcodec/hevc_parser.c
> @@ -42,7 +42,6 @@ typedef struct HEVCParserContext {
>      H2645Packet pkt;
>      HEVCParamSets ps;
>      HEVCSEI sei;
> -    SliceHeader sh;
>  
>      int is_avc;
>      int nal_length_size;
> @@ -58,26 +57,28 @@ static int hevc_parse_slice_header(AVCodecParserContext *s, H2645NAL *nal,
>      HEVCParserContext *ctx = s->priv_data;
>      HEVCParamSets *ps = &ctx->ps;
>      HEVCSEI *sei = &ctx->sei;
> -    SliceHeader *sh = &ctx->sh;
>      GetBitContext *gb = &nal->gb;
>      const HEVCWindow *ow;
>      int i, num = 0, den = 0;
>  
> -    sh->first_slice_in_pic_flag = get_bits1(gb);
> +    unsigned int pps_id, first_slice_in_pic_flag, dependent_slice_segment_flag;
> +    enum HEVCSliceType slice_type;
> +
> +    first_slice_in_pic_flag = get_bits1(gb);
>      s->picture_structure = sei->picture_timing.picture_struct;
>      s->field_order = sei->picture_timing.picture_struct;
>  
>      if (IS_IRAP_NAL(nal)) {
>          s->key_frame = 1;
> -        sh->no_output_of_prior_pics_flag = get_bits1(gb);
> +        skip_bits1(gb); // no_output_of_prior_pics_flag
>      }
>  
> -    sh->pps_id = get_ue_golomb(gb);
> -    if (sh->pps_id >= HEVC_MAX_PPS_COUNT || !ps->pps_list[sh->pps_id]) {
> -        av_log(avctx, AV_LOG_ERROR, "PPS id out of range: %d\n", sh->pps_id);
> +    pps_id = get_ue_golomb(gb);
> +    if (pps_id >= HEVC_MAX_PPS_COUNT || !ps->pps_list[pps_id]) {
> +        av_log(avctx, AV_LOG_ERROR, "PPS id out of range: %d\n", pps_id);
>          return AVERROR_INVALIDDATA;
>      }
> -    ps->pps = (HEVCPPS*)ps->pps_list[sh->pps_id]->data;
> +    ps->pps = (HEVCPPS*)ps->pps_list[pps_id]->data;
>  
>      if (ps->pps->sps_id >= HEVC_MAX_SPS_COUNT || !ps->sps_list[ps->pps->sps_id]) {
>          av_log(avctx, AV_LOG_ERROR, "SPS id out of range: %d\n", ps->pps->sps_id);
> @@ -109,51 +110,53 @@ static int hevc_parse_slice_header(AVCodecParserContext *s, H2645NAL *nal,
>          av_reduce(&avctx->framerate.den, &avctx->framerate.num,
>                    num, den, 1 << 30);
>  
> -    if (!sh->first_slice_in_pic_flag) {
> +    if (!first_slice_in_pic_flag) {
> +        unsigned int slice_segment_addr;
>          int slice_address_length;
>  
>          if (ps->pps->dependent_slice_segments_enabled_flag)
> -            sh->dependent_slice_segment_flag = get_bits1(gb);
> +            dependent_slice_segment_flag = get_bits1(gb);
>          else
> -            sh->dependent_slice_segment_flag = 0;
> +            dependent_slice_segment_flag = 0;
>  
>          slice_address_length = av_ceil_log2_c(ps->sps->ctb_width *
>                                                ps->sps->ctb_height);
> -        sh->slice_segment_addr = get_bitsz(gb, slice_address_length);
> -        if (sh->slice_segment_addr >= ps->sps->ctb_width * ps->sps->ctb_height) {
> +        slice_segment_addr = get_bitsz(gb, slice_address_length);
> +        if (slice_segment_addr >= ps->sps->ctb_width * ps->sps->ctb_height) {
>              av_log(avctx, AV_LOG_ERROR, "Invalid slice segment address: %u.\n",
> -                   sh->slice_segment_addr);
> +                   slice_segment_addr);
>              return AVERROR_INVALIDDATA;
>          }
>      } else
> -        sh->dependent_slice_segment_flag = 0;
> +        dependent_slice_segment_flag = 0;
>  
> -    if (sh->dependent_slice_segment_flag)
> +    if (dependent_slice_segment_flag)
>          return 0; /* break; */
>  
>      for (i = 0; i < ps->pps->num_extra_slice_header_bits; i++)
>          skip_bits(gb, 1); // slice_reserved_undetermined_flag[]
>  
> -    sh->slice_type = get_ue_golomb(gb);
> -    if (!(sh->slice_type == HEVC_SLICE_I || sh->slice_type == HEVC_SLICE_P ||
> -          sh->slice_type == HEVC_SLICE_B)) {
> +    slice_type = get_ue_golomb(gb);
> +    if (!(slice_type == HEVC_SLICE_I || slice_type == HEVC_SLICE_P ||
> +          slice_type == HEVC_SLICE_B)) {
>          av_log(avctx, AV_LOG_ERROR, "Unknown slice type: %d.\n",
> -               sh->slice_type);
> +               slice_type);
>          return AVERROR_INVALIDDATA;
>      }
> -    s->pict_type = sh->slice_type == HEVC_SLICE_B ? AV_PICTURE_TYPE_B :
> -                   sh->slice_type == HEVC_SLICE_P ? AV_PICTURE_TYPE_P :
> -                                               AV_PICTURE_TYPE_I;
> +    s->pict_type = slice_type == HEVC_SLICE_B ? AV_PICTURE_TYPE_B :
> +                   slice_type == HEVC_SLICE_P ? AV_PICTURE_TYPE_P :
> +                                                AV_PICTURE_TYPE_I;
>  
>      if (ps->pps->output_flag_present_flag)
> -        sh->pic_output_flag = get_bits1(gb);
> +        skip_bits1(gb); // pic_output_flag
>  
>      if (ps->sps->separate_colour_plane_flag)
> -        sh->colour_plane_id = get_bits(gb, 2);
> +        skip_bits(gb, 2);   // colour_plane_id
>  
>      if (!IS_IDR_NAL(nal)) {
> -        sh->pic_order_cnt_lsb = get_bits(gb, ps->sps->log2_max_poc_lsb);
> -        s->output_picture_number = ctx->poc = ff_hevc_compute_poc(ps->sps, ctx->pocTid0, sh->pic_order_cnt_lsb, nal->type);
> +        int pic_order_cnt_lsb = get_bits(gb, ps->sps->log2_max_poc_lsb);
> +        s->output_picture_number = ctx->poc =
> +            ff_hevc_compute_poc(ps->sps, ctx->pocTid0, pic_order_cnt_lsb, nal->type);
>      } else
>          s->output_picture_number = ctx->poc = 0;
>  
> diff --git a/libavcodec/hevc_ps.h b/libavcodec/hevc_ps.h
> index 238edd3ddc..2b3614482e 100644
> --- a/libavcodec/hevc_ps.h
> +++ b/libavcodec/hevc_ps.h
> @@ -46,83 +46,6 @@ typedef struct LongTermRPS {
>      uint8_t nb_refs;
>  } LongTermRPS;

LongTermRPS should be moved back as well. And it could all be done as a
git revert 4aaace8b25 commit instead, immediately after you removed
SliceHeader usage from hevc_parser.

LGTM either way. Moving SliceHeader to hevc_ps.h was one of the things i
wasn't happy about when i decoupled the parser from the decoder back then.
diff mbox series

Patch

diff --git a/libavcodec/hevc_parser.c b/libavcodec/hevc_parser.c
index 84f19b485c..5af4b788d5 100644
--- a/libavcodec/hevc_parser.c
+++ b/libavcodec/hevc_parser.c
@@ -42,7 +42,6 @@  typedef struct HEVCParserContext {
     H2645Packet pkt;
     HEVCParamSets ps;
     HEVCSEI sei;
-    SliceHeader sh;
 
     int is_avc;
     int nal_length_size;
@@ -58,26 +57,28 @@  static int hevc_parse_slice_header(AVCodecParserContext *s, H2645NAL *nal,
     HEVCParserContext *ctx = s->priv_data;
     HEVCParamSets *ps = &ctx->ps;
     HEVCSEI *sei = &ctx->sei;
-    SliceHeader *sh = &ctx->sh;
     GetBitContext *gb = &nal->gb;
     const HEVCWindow *ow;
     int i, num = 0, den = 0;
 
-    sh->first_slice_in_pic_flag = get_bits1(gb);
+    unsigned int pps_id, first_slice_in_pic_flag, dependent_slice_segment_flag;
+    enum HEVCSliceType slice_type;
+
+    first_slice_in_pic_flag = get_bits1(gb);
     s->picture_structure = sei->picture_timing.picture_struct;
     s->field_order = sei->picture_timing.picture_struct;
 
     if (IS_IRAP_NAL(nal)) {
         s->key_frame = 1;
-        sh->no_output_of_prior_pics_flag = get_bits1(gb);
+        skip_bits1(gb); // no_output_of_prior_pics_flag
     }
 
-    sh->pps_id = get_ue_golomb(gb);
-    if (sh->pps_id >= HEVC_MAX_PPS_COUNT || !ps->pps_list[sh->pps_id]) {
-        av_log(avctx, AV_LOG_ERROR, "PPS id out of range: %d\n", sh->pps_id);
+    pps_id = get_ue_golomb(gb);
+    if (pps_id >= HEVC_MAX_PPS_COUNT || !ps->pps_list[pps_id]) {
+        av_log(avctx, AV_LOG_ERROR, "PPS id out of range: %d\n", pps_id);
         return AVERROR_INVALIDDATA;
     }
-    ps->pps = (HEVCPPS*)ps->pps_list[sh->pps_id]->data;
+    ps->pps = (HEVCPPS*)ps->pps_list[pps_id]->data;
 
     if (ps->pps->sps_id >= HEVC_MAX_SPS_COUNT || !ps->sps_list[ps->pps->sps_id]) {
         av_log(avctx, AV_LOG_ERROR, "SPS id out of range: %d\n", ps->pps->sps_id);
@@ -109,51 +110,53 @@  static int hevc_parse_slice_header(AVCodecParserContext *s, H2645NAL *nal,
         av_reduce(&avctx->framerate.den, &avctx->framerate.num,
                   num, den, 1 << 30);
 
-    if (!sh->first_slice_in_pic_flag) {
+    if (!first_slice_in_pic_flag) {
+        unsigned int slice_segment_addr;
         int slice_address_length;
 
         if (ps->pps->dependent_slice_segments_enabled_flag)
-            sh->dependent_slice_segment_flag = get_bits1(gb);
+            dependent_slice_segment_flag = get_bits1(gb);
         else
-            sh->dependent_slice_segment_flag = 0;
+            dependent_slice_segment_flag = 0;
 
         slice_address_length = av_ceil_log2_c(ps->sps->ctb_width *
                                               ps->sps->ctb_height);
-        sh->slice_segment_addr = get_bitsz(gb, slice_address_length);
-        if (sh->slice_segment_addr >= ps->sps->ctb_width * ps->sps->ctb_height) {
+        slice_segment_addr = get_bitsz(gb, slice_address_length);
+        if (slice_segment_addr >= ps->sps->ctb_width * ps->sps->ctb_height) {
             av_log(avctx, AV_LOG_ERROR, "Invalid slice segment address: %u.\n",
-                   sh->slice_segment_addr);
+                   slice_segment_addr);
             return AVERROR_INVALIDDATA;
         }
     } else
-        sh->dependent_slice_segment_flag = 0;
+        dependent_slice_segment_flag = 0;
 
-    if (sh->dependent_slice_segment_flag)
+    if (dependent_slice_segment_flag)
         return 0; /* break; */
 
     for (i = 0; i < ps->pps->num_extra_slice_header_bits; i++)
         skip_bits(gb, 1); // slice_reserved_undetermined_flag[]
 
-    sh->slice_type = get_ue_golomb(gb);
-    if (!(sh->slice_type == HEVC_SLICE_I || sh->slice_type == HEVC_SLICE_P ||
-          sh->slice_type == HEVC_SLICE_B)) {
+    slice_type = get_ue_golomb(gb);
+    if (!(slice_type == HEVC_SLICE_I || slice_type == HEVC_SLICE_P ||
+          slice_type == HEVC_SLICE_B)) {
         av_log(avctx, AV_LOG_ERROR, "Unknown slice type: %d.\n",
-               sh->slice_type);
+               slice_type);
         return AVERROR_INVALIDDATA;
     }
-    s->pict_type = sh->slice_type == HEVC_SLICE_B ? AV_PICTURE_TYPE_B :
-                   sh->slice_type == HEVC_SLICE_P ? AV_PICTURE_TYPE_P :
-                                               AV_PICTURE_TYPE_I;
+    s->pict_type = slice_type == HEVC_SLICE_B ? AV_PICTURE_TYPE_B :
+                   slice_type == HEVC_SLICE_P ? AV_PICTURE_TYPE_P :
+                                                AV_PICTURE_TYPE_I;
 
     if (ps->pps->output_flag_present_flag)
-        sh->pic_output_flag = get_bits1(gb);
+        skip_bits1(gb); // pic_output_flag
 
     if (ps->sps->separate_colour_plane_flag)
-        sh->colour_plane_id = get_bits(gb, 2);
+        skip_bits(gb, 2);   // colour_plane_id
 
     if (!IS_IDR_NAL(nal)) {
-        sh->pic_order_cnt_lsb = get_bits(gb, ps->sps->log2_max_poc_lsb);
-        s->output_picture_number = ctx->poc = ff_hevc_compute_poc(ps->sps, ctx->pocTid0, sh->pic_order_cnt_lsb, nal->type);
+        int pic_order_cnt_lsb = get_bits(gb, ps->sps->log2_max_poc_lsb);
+        s->output_picture_number = ctx->poc =
+            ff_hevc_compute_poc(ps->sps, ctx->pocTid0, pic_order_cnt_lsb, nal->type);
     } else
         s->output_picture_number = ctx->poc = 0;
 
diff --git a/libavcodec/hevc_ps.h b/libavcodec/hevc_ps.h
index 238edd3ddc..2b3614482e 100644
--- a/libavcodec/hevc_ps.h
+++ b/libavcodec/hevc_ps.h
@@ -46,83 +46,6 @@  typedef struct LongTermRPS {
     uint8_t nb_refs;
 } LongTermRPS;
 
-typedef struct SliceHeader {
-    unsigned int pps_id;
-
-    ///< address (in raster order) of the first block in the current slice segment
-    unsigned int   slice_segment_addr;
-    ///< address (in raster order) of the first block in the current slice
-    unsigned int   slice_addr;
-
-    enum HEVCSliceType slice_type;
-
-    int pic_order_cnt_lsb;
-
-    uint8_t first_slice_in_pic_flag;
-    uint8_t dependent_slice_segment_flag;
-    uint8_t pic_output_flag;
-    uint8_t colour_plane_id;
-
-    ///< RPS coded in the slice header itself is stored here
-    int short_term_ref_pic_set_sps_flag;
-    int short_term_ref_pic_set_size;
-    ShortTermRPS slice_rps;
-    const ShortTermRPS *short_term_rps;
-    int long_term_ref_pic_set_size;
-    LongTermRPS long_term_rps;
-    unsigned int list_entry_lx[2][32];
-
-    uint8_t rpl_modification_flag[2];
-    uint8_t no_output_of_prior_pics_flag;
-    uint8_t slice_temporal_mvp_enabled_flag;
-
-    unsigned int nb_refs[2];
-
-    uint8_t slice_sample_adaptive_offset_flag[3];
-    uint8_t mvd_l1_zero_flag;
-
-    uint8_t cabac_init_flag;
-    uint8_t disable_deblocking_filter_flag; ///< slice_header_disable_deblocking_filter_flag
-    uint8_t slice_loop_filter_across_slices_enabled_flag;
-    uint8_t collocated_list;
-
-    unsigned int collocated_ref_idx;
-
-    int slice_qp_delta;
-    int slice_cb_qp_offset;
-    int slice_cr_qp_offset;
-
-    uint8_t cu_chroma_qp_offset_enabled_flag;
-
-    int beta_offset;    ///< beta_offset_div2 * 2
-    int tc_offset;      ///< tc_offset_div2 * 2
-
-    unsigned int max_num_merge_cand; ///< 5 - 5_minus_max_num_merge_cand
-
-    unsigned *entry_point_offset;
-    int * offset;
-    int * size;
-    int num_entry_point_offsets;
-
-    int8_t slice_qp;
-
-    uint8_t luma_log2_weight_denom;
-    int16_t chroma_log2_weight_denom;
-
-    int16_t luma_weight_l0[16];
-    int16_t chroma_weight_l0[16][2];
-    int16_t chroma_weight_l1[16][2];
-    int16_t luma_weight_l1[16];
-
-    int16_t luma_offset_l0[16];
-    int16_t chroma_offset_l0[16][2];
-
-    int16_t luma_offset_l1[16];
-    int16_t chroma_offset_l1[16][2];
-
-    int slice_ctb_addr_rs;
-} SliceHeader;
-
 typedef struct HEVCWindow {
     unsigned int left_offset;
     unsigned int right_offset;
diff --git a/libavcodec/hevcdec.h b/libavcodec/hevcdec.h
index 89e0809850..d9de46f89d 100644
--- a/libavcodec/hevcdec.h
+++ b/libavcodec/hevcdec.h
@@ -337,6 +337,83 @@  typedef struct HEVCFrame {
     uint8_t flags;
 } HEVCFrame;
 
+typedef struct SliceHeader {
+    unsigned int pps_id;
+
+    ///< address (in raster order) of the first block in the current slice segment
+    unsigned int   slice_segment_addr;
+    ///< address (in raster order) of the first block in the current slice
+    unsigned int   slice_addr;
+
+    enum HEVCSliceType slice_type;
+
+    int pic_order_cnt_lsb;
+
+    uint8_t first_slice_in_pic_flag;
+    uint8_t dependent_slice_segment_flag;
+    uint8_t pic_output_flag;
+    uint8_t colour_plane_id;
+
+    ///< RPS coded in the slice header itself is stored here
+    int short_term_ref_pic_set_sps_flag;
+    int short_term_ref_pic_set_size;
+    ShortTermRPS slice_rps;
+    const ShortTermRPS *short_term_rps;
+    int long_term_ref_pic_set_size;
+    LongTermRPS long_term_rps;
+    unsigned int list_entry_lx[2][32];
+
+    uint8_t rpl_modification_flag[2];
+    uint8_t no_output_of_prior_pics_flag;
+    uint8_t slice_temporal_mvp_enabled_flag;
+
+    unsigned int nb_refs[2];
+
+    uint8_t slice_sample_adaptive_offset_flag[3];
+    uint8_t mvd_l1_zero_flag;
+
+    uint8_t cabac_init_flag;
+    uint8_t disable_deblocking_filter_flag; ///< slice_header_disable_deblocking_filter_flag
+    uint8_t slice_loop_filter_across_slices_enabled_flag;
+    uint8_t collocated_list;
+
+    unsigned int collocated_ref_idx;
+
+    int slice_qp_delta;
+    int slice_cb_qp_offset;
+    int slice_cr_qp_offset;
+
+    uint8_t cu_chroma_qp_offset_enabled_flag;
+
+    int beta_offset;    ///< beta_offset_div2 * 2
+    int tc_offset;      ///< tc_offset_div2 * 2
+
+    unsigned int max_num_merge_cand; ///< 5 - 5_minus_max_num_merge_cand
+
+    unsigned *entry_point_offset;
+    int * offset;
+    int * size;
+    int num_entry_point_offsets;
+
+    int8_t slice_qp;
+
+    uint8_t luma_log2_weight_denom;
+    int16_t chroma_log2_weight_denom;
+
+    int16_t luma_weight_l0[16];
+    int16_t chroma_weight_l0[16][2];
+    int16_t chroma_weight_l1[16][2];
+    int16_t luma_weight_l1[16];
+
+    int16_t luma_offset_l0[16];
+    int16_t chroma_offset_l0[16][2];
+
+    int16_t luma_offset_l1[16];
+    int16_t chroma_offset_l1[16][2];
+
+    int slice_ctb_addr_rs;
+} SliceHeader;
+
 typedef struct HEVCLocalContext {
     uint8_t cabac_state[HEVC_CONTEXTS];