[FFmpeg-devel] [PATCH V5] avcodec/libvpxenc: add ROI-based encoding support for VP8/VP9 support

[James Zern]

On Wed, Aug 28, 2019 at 10:20 PM Guo, Yejun <yejun.guo at intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: James Zern [mailto:jzern at google.com]
> > Sent: Thursday, August 29, 2019 12:39 PM
> > To: Guo, Yejun <yejun.guo at intel.com>
> > Cc: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH V5] avcodec/libvpxenc: add ROI-based
> > encoding support for VP8/VP9 support
> >
> > Hi,
> >
> > On Mon, Aug 26, 2019 at 11:30 PM Guo, Yejun <yejun.guo at intel.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: James Zern [mailto:jzern at google.com]
> > > > Sent: Tuesday, August 27, 2019 12:03 PM
> > > > To: FFmpeg development discussions and patches
> > <ffmpeg-devel at ffmpeg.org>
> > > > Cc: Guo, Yejun <yejun.guo at intel.com>
> > > > Subject: Re: [FFmpeg-devel] [PATCH V5] avcodec/libvpxenc: add ROI-based
> > > > encoding support for VP8/VP9 support
> > > >
> > > > Hi,
> > > >
> > > > On Thu, Aug 22, 2019 at 5:56 PM Guo, Yejun <yejun.guo at intel.com> wrote:
> > > > >
> > > > > example command line to verify it:
> > > > > ./ffmpeg -i input.stream -vf addroi=0:0:iw/3:ih/3:-0.8 -c:v libvpx -b:v 2M
> > > > tmp.webm
> > > > >
> > > > > Signed-off-by: Guo, Yejun <yejun.guo at intel.com>
> > > > > ---
> > > > >  libavcodec/libvpxenc.c | 194
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 194 insertions(+)
> > > > >
> > > >
> > > > Looks OK overall, just a few minor comments and you should bump the
> > > > micro version number for this change.
> > >
> > > thanks,
> > > do you mean bump LIBAVCODEC_VERSION_MICRO in file
> > libavcodec/version.h? thanks.
> > >
> >
> > Yes.
> >
> > > >
> > > > > [...]
> > > > > +
> > > > > +            segment_mapping[mapping_index] = segment_id + 1;
> > > > > +            roi_map->delta_q[segment_id] = delta_q;
> > > > > +            segment_id++;
> > > > > +        }
> > > > > +    }
> > > > > +
> > > > > +
> > > >
> > > > This line can go.
> > >
> > > do you mean the next line av_assert0 can be removed? thanks.
> > >
> >
> > No, I meant the extra blank line, there are 2 of them.
>
> My intention for the two blank lines was to divide the two logical parts more clear. Anyway, I'll follow to remove.
>

If there's a reason to divide them then a separate function or comment
for the next section could work. Extra blank lines are likely to be
cleaned up by tools or other developers.

> and I've send out v7 for this patch this morning, do you have any other comment? thanks. So I can combine all the comments in v8 patch.
>

I think the last set was all I had, though I could give it more testing.