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.fo...@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...@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.fo...@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.
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
