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

[Michael Niedermayer]

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

[...]

thx


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160623/e0648901/attachment.sig>