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

[Benoit Fouet]

Hi,

On 23/06/2016 22:37, Michael Niedermayer wrote:
> On Thu, Jun 23, 2016 at 03:28:10PM +0200, Benoit Fouet wrote:
>> 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?
> iam ok with the patch, maybe give others a bit of time to reply
> before applying though

Yeah, I'm in no hurry, I just saw this FIXME in the context of one of 
Mateo's patches.
I was more waiting for some feedback from Hendrik or Clément, anyway, as 
they were the ones raising the points.

-- 
Ben