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

[Guo, Yejun]

> -----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.

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.