[FFmpeg-devel] [GSOC][PATCH 1/3] lavc/cfhd:3d transform decoding for both progressive and interlaced

[Michael Niedermayer]

On Fri, Aug 17, 2018 at 11:45:04AM +0530, Gagandeep Singh wrote:
> On Wed, Aug 15, 2018 at 3:12 AM Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > On Tue, Aug 14, 2018 at 01:12:37PM +0530, Gagandeep Singh wrote:
> > > IP sample decoding patch attached herein.
> > >
> > > Gagandeep Singh
> >
> > >  cfhd.c |  511
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
> > >  cfhd.h |   13 +
> > >  2 files changed, 454 insertions(+), 70 deletions(-)
> > > a155a004ae249d84063d9c49effb87a8f98b0fe7  patchip.patch
> > > From 6cc5636c48bca4e802ccca5f53560e31360760cb Mon Sep 17 00:00:00 2001
> > > From: Gagandeep Singh <deepgagan231197 at gmail.com>
> > > Date: Tue, 14 Aug 2018 00:07:45 +0530
> > > Subject: [GSOC][FFmpeg-devel][PATCH 1/3] lavc/cfhd:3d transform decoding
> > for both progressive and
> > >  interlaced
> > >
> > [...]
> > > @@ -420,9 +486,15 @@ static int cfhd_decode(AVCodecContext *avctx, void
> > *data, int *got_frame,
> > >              s->prescale_shift[1] = (data >> 3) & 0x7;
> > >              s->prescale_shift[2] = (data >> 6) & 0x7;
> > >              av_log(avctx, AV_LOG_DEBUG, "Prescale shift (VC-5): %x\n",
> > data);
> > > +        } else if (tag == EncodingMethod) {
> > > +            s->encode_method = data;
> > > +            av_log(avctx, AV_LOG_DEBUG, "Encode Method for Subband %d :
> > %x\n",s->subband_num_actual, data);
> >
> > >          } else if (tag == 27) {
> > >              av_log(avctx, AV_LOG_DEBUG, "Lowpass width %"PRIu16"\n",
> > data);
> > > -            if (data < 3 || data >
> > s->plane[s->channel_num].band[0][0].a_width) {
> > > +            if (s->coded_width == 0){
> > > +                s->coded_width = data << 3;
> > > +              }
> > > +                if (data < 3) {
> > >                  av_log(avctx, AV_LOG_ERROR, "Invalid lowpass width\n");
> > >                  ret = AVERROR(EINVAL);
> >
> > any checks should be done before the variable is used.
> > also teh indention looks rather odd here
> >
> >
> > [...]
> > > @@ -664,9 +742,14 @@ static int cfhd_decode(AVCodecContext *avctx, void
> > *data, int *got_frame,
> > >                          if (count > expected)
> > >                              break;
> > >
> > > -                        coeff = dequant_and_decompand(level,
> > s->quantisation, 0);
> > > +                        coeff = dequant_and_decompand(level,
> > s->quantisation, 0, (s->sample_type == 2 || s->sample_type == 3) &&
> > s->pframe && s->subband_num_actual == 7 && s->encode_method == 5);
> > >                          for (i = 0; i < run; i++)
> > > -                            *coeff_data++ = coeff;
> > > +                            if (tag != 82)
> >
> > what is tag 82 ?
> > i think instead of litteral numbers named identifers should be used
> >
> 
> tag 82 is to tell the decoder to use the next coefficients for the same
> subband, i.e, process the coefficients in the latest subband with the new
> coefficients.
> Yes, you are right, i should have used name instead of number, i can send
> the patch again but i would also have to send the other patches again. Or i
> can send a patch to be applied on top of the 3 patches i sent. Please
> comment.

once there are "enough comments" so most patches need changes or theres a
absence of  further comments repost with all comments taken care of


> 
> >
> >
> 
> > [...]
> > > @@ -726,14 +814,15 @@ static int cfhd_decode(AVCodecContext *avctx, void
> > *data, int *got_frame,
> > >              }
> > >          }
> > >      }
> > > -
> > > -    if (!s->a_width || !s->a_height || s->a_format == AV_PIX_FMT_NONE ||
> > > -        s->coded_width || s->coded_height || s->coded_format !=
> > AV_PIX_FMT_NONE) {
> >
> > > +    //disabled to run mountain sample file
> > > +#if 0
> > > +    if ((!s->a_width || !s->a_height || s->a_format == AV_PIX_FMT_NONE
> > ||
> > > +        s->coded_width || s->coded_height || s->coded_format !=
> > AV_PIX_FMT_NONE) && s->sample_type != 1) {
> > >          av_log(avctx, AV_LOG_ERROR, "Invalid dimensions\n");
> > >          ret = AVERROR(EINVAL);
> > >          goto end;
> > >      }
> > > -
> > > +#endif
> >
> > please elaborate why this needs to be disabled
> > i presume this code was needed for something
> >
> I didn't need to disable this for any sample except one, where the image
> height and width data wasn't transfered in accordance to how it was in the
> rest of the sample so the flow of code was just causing the decoder to
> crash. I can produce a more robust fix, though again will need to repost
> other patches as well, please comment.

yes, "more robust fix" sounds better than a random check commented out

[...]

thx

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180817/95b1a6ef/attachment.sig>