[FFmpeg-devel] [PATCH] avcodec/hevc_ps: Move return code setting code to the error cases

[James Almer]

On 09/07/15 12:26 AM, Michael Niedermayer wrote:
> Iam not sure this is better or worse than before
> What do people prefer ?
> 
> Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> ---
>  libavcodec/hevc_ps.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> index 1038516..372081a 100644
> --- a/libavcodec/hevc_ps.c
> +++ b/libavcodec/hevc_ps.c
> @@ -998,7 +998,6 @@ int ff_hevc_parse_sps(HEVCSPS *sps, GetBitContext *gb, unsigned int *sps_id,
>                                                   sps, 0)) < 0)
>              goto err;
>      }
> -    ret = AVERROR_INVALIDDATA;
>  
>      sps->long_term_ref_pics_present_flag = get_bits1(gb);
>      if (sps->long_term_ref_pics_present_flag) {
> @@ -1006,6 +1005,7 @@ int ff_hevc_parse_sps(HEVCSPS *sps, GetBitContext *gb, unsigned int *sps_id,
>          if (sps->num_long_term_ref_pics_sps > 31U) {
>              av_log(avctx, AV_LOG_ERROR, "num_long_term_ref_pics_sps %d is out of range.\n",
>                     sps->num_long_term_ref_pics_sps);
> +            ret = AVERROR_INVALIDDATA;
>              goto err;
>          }
>          for (i = 0; i < sps->num_long_term_ref_pics_sps; i++) {
> @@ -1096,6 +1096,7 @@ int ff_hevc_parse_sps(HEVCSPS *sps, GetBitContext *gb, unsigned int *sps_id,
>  
>      if (sps->log2_ctb_size > MAX_LOG2_CTB_SIZE) {
>          av_log(avctx, AV_LOG_ERROR, "CTB size out of range: 2^%d\n", sps->log2_ctb_size);
> +        ret = AVERROR_INVALIDDATA;
>          goto err;
>      }
>      if (sps->log2_ctb_size < 4) {
> @@ -1104,6 +1105,7 @@ int ff_hevc_parse_sps(HEVCSPS *sps, GetBitContext *gb, unsigned int *sps_id,
>                 "log2_ctb_size %d differs from the bounds of any known profile\n",
>                 sps->log2_ctb_size);
>          avpriv_request_sample(avctx, "log2_ctb_size %d", sps->log2_ctb_size);
> +        ret = AVERROR_INVALIDDATA;
>          goto err;
>      }
>  
> @@ -1124,29 +1126,34 @@ int ff_hevc_parse_sps(HEVCSPS *sps, GetBitContext *gb, unsigned int *sps_id,
>      if (av_mod_uintp2(sps->width, sps->log2_min_cb_size) ||
>          av_mod_uintp2(sps->height, sps->log2_min_cb_size)) {
>          av_log(avctx, AV_LOG_ERROR, "Invalid coded frame dimensions.\n");
> +        ret = AVERROR_INVALIDDATA;
>          goto err;
>      }
>  
>      if (sps->max_transform_hierarchy_depth_inter > sps->log2_ctb_size - sps->log2_min_tb_size) {
>          av_log(avctx, AV_LOG_ERROR, "max_transform_hierarchy_depth_inter out of range: %d\n",
>                 sps->max_transform_hierarchy_depth_inter);
> +        ret = AVERROR_INVALIDDATA;
>          goto err;
>      }
>      if (sps->max_transform_hierarchy_depth_intra > sps->log2_ctb_size - sps->log2_min_tb_size) {
>          av_log(avctx, AV_LOG_ERROR, "max_transform_hierarchy_depth_intra out of range: %d\n",
>                 sps->max_transform_hierarchy_depth_intra);
> +        ret = AVERROR_INVALIDDATA;
>          goto err;
>      }
>      if (sps->log2_max_trafo_size > FFMIN(sps->log2_ctb_size, 5)) {
>          av_log(avctx, AV_LOG_ERROR,
>                 "max transform block size out of range: %d\n",
>                 sps->log2_max_trafo_size);
> +        ret = AVERROR_INVALIDDATA;
>          goto err;
>      }
>  
>      if (get_bits_left(gb) < 0) {
>          av_log(avctx, AV_LOG_ERROR,
>                 "Overread SPS by %d bits\n", -get_bits_left(gb));
> +        ret = AVERROR_INVALIDDATA;
>          goto err;
>      }
>  

Why not just replace every "goto err" with a return AVERROR_INVALIDDATA? The gotos
seem pointless considering the only code after it is "return ret". No memory being
freed and nothing being uninitialized to justify them.