[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 2:14 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
> 
> 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.

got it, thanks.