[FFmpeg-devel] [PATCH] h264: make H264ParamSets sps const

[Benoit Fouet]

Hi,

On 21/06/2016 16:29, Hendrik Leppkes wrote:
> On Tue, Jun 21, 2016 at 4:20 PM, Benoit Fouet <benoit.fouet at free.fr> wrote:
>> Hi,
>>
>>
>> On 21/06/2016 14:52, Hendrik Leppkes wrote:
>>> On Tue, Jun 21, 2016 at 2:40 PM, Clément Bœsch <u at pkh.me> wrote:
>>>> On Tue, Jun 21, 2016 at 02:34:33PM +0200, Benoit Fouet wrote:
>>>>> Hi,
>>>>>
>>>>> Unless I totally missed something, the FIXME in H264ParamSets structure
>>>>> should be fixed by attached patch.
>>>>>
>>>>> --
>>>>> Ben
>>>>>
>>>>>   From 28ae10498f81070539bdb8f40236326743350101 Mon Sep 17 00:00:00 2001
>>>>> From: Benoit Fouet <benoit.fouet at free.fr>
>>>>> Date: Tue, 21 Jun 2016 14:17:13 +0200
>>>>> Subject: [PATCH] h264: make H264ParamSets sps const
>>>>>
>>>>> ---
>>>>>    libavcodec/h264.h       | 3 +--
>>>>>    libavcodec/h264_slice.c | 2 +-
>>>>>    2 files changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/libavcodec/h264.h b/libavcodec/h264.h
>>>>> index c4d2921..b809ee5 100644
>>>>> --- a/libavcodec/h264.h
>>>>> +++ b/libavcodec/h264.h
>>>>> @@ -234,8 +234,7 @@ typedef struct H264ParamSets {
>>>>>        AVBufferRef *sps_ref;
>>>>>        /* currently active parameters sets */
>>>>>        const PPS *pps;
>>>>> -    // FIXME this should properly be const
>>>>> -    SPS *sps;
>>>>> +    const SPS *sps;
>>>>>    } H264ParamSets;
>>>>>
>>>>>    /**
>>>>> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
>>>>> index 6e7b940..da7f9dd 100644
>>>>> --- a/libavcodec/h264_slice.c
>>>>> +++ b/libavcodec/h264_slice.c
>>>>> @@ -873,7 +873,7 @@ static enum AVPixelFormat
>>>>> get_pixel_format(H264Context *h, int force_callback)
>>>>>    /* export coded and cropped frame dimensions to AVCodecContext */
>>>>>    static int init_dimensions(H264Context *h)
>>>>>    {
>>>>> -    SPS *sps = h->ps.sps;
>>>>> +    SPS *sps = (SPS*)h->ps.sps_ref->data;
>>>>>        int width  = h->width  - (sps->crop_right + sps->crop_left);
>>>>>        int height = h->height - (sps->crop_top   + sps->crop_bottom);
>>>>>        av_assert0(sps->crop_right + sps->crop_left < (unsigned)h->width);
>>>> So it's not actually const, right?
>>>>
>>> Indeed, the FIXME wasn't just there because someone forgot to write
>>> "const" in front of it, but because it was used in some parts as
>>> not-const.
>>
>> OK, right... Thanks for reminding me of reading the code better before
>> sending a patch.
>>
>> As far as I can see, the only place where this constness is not preserved is
>> in the init_dimensions function (in h264_slice), in a dead part of the code,
>> as crop is asserted at the beginning of the very same function.
>> Please correct me if I've missed other places.
>>
> If anything the asserts should probably be removed, because bad files
> should never be able to trigger assertions, and the existing check
> remain.

Well, the SPS "decoder" already takes care of the check (see 
ff_h264_decode_seq_parameter_set).
So I could remove the check, because it seems useless, instead of 
removing it because "bad things happen", what do you think?

Thanks,
-- 
Ben