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

[Gagandeep Singh]

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.

>
>

> [...]
> > @@ -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.

> also this decoder with the patches should be tested with a fuzzer to
> reduce
> the chance of bugs
>
> I don't know how to use 'fuzzer', sorry, though i can look into that.

> also i saw on the IRC log after you logged off
> <gagandeepsingh> also how do i send the 3 samples i have
> <kierank> ask michaelni
> <kierank> michaelni: can you help gagandeepsingh
>
> i didnt follow the whole discussion but to submit samples, you can upload
> them
> somewhere and post a link. Someone developer will upload them.
> Note, please clearly specify what any samples are for (fatesamples / bug
> reports / other /...)
>
> The samples are proper bitexact IP decoded samples and i believe they
should be added as fate tests

> thanks
>
[...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Whats the most studid thing your enemy could do ? Blow himself up
> Whats the most studid thing you could do ? Give up your rights and
> freedom because your enemy blew himself up.

Thanks for reviewing.

Gagandeep Singh

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>