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

[Benoit Fouet]

Hi,


On 21/06/2016 16:42, Benoit Fouet wrote:
> 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?
>

Any objection to this patch now?

Thanks,
-- 
Ben