[FFmpeg-devel] [PATCH V8 2/2] avcodec/libx264: add support for ROI-based encoding

Guo, Yejun yejun.guo at intel.com
Wed Jan 16 02:41:35 EET 2019


this patch set asks for pushing if no more concerns, thanks.

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Guo, Yejun
> Sent: Friday, January 11, 2019 9:39 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH V8 2/2] avcodec/libx264: add support
> for ROI-based encoding
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> Behalf
> > Of Carl Eugen Hoyos
> > Sent: Friday, January 11, 2019 8:54 AM
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel at ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH V8 2/2] avcodec/libx264: add
> > support for ROI-based encoding
> >
> > 2019-01-11 1:37 GMT+01:00, Guo, Yejun <yejun.guo at intel.com>:
> >
> > >> 2019-01-10 9:54 GMT+01:00, Guo, Yejun <yejun.guo at intel.com>:
> > >>
> > >> > +                        roi = (AVRegionOfInterest*)((char*)roi +
> > >> > roi->self_size);
> > >>
> > >> Isn't this roi++?
> > >> If yes, please change this.
> > >
> > > no, it's not roi++, the reason is that struct AVRegionOfInterest
> > > might be extended, so to keep ABI compatible, we add the
> > > 'self_size'.  It is discussed in V4 comments.
> >
> > So after AVRegionOfInterest was extended, you as a user who had
> > compiled his application against old libavcodec will use new
> > libavcodec (with the
> > extension) and it will be guaranteed that libx264 will still get the
> > correct information although only the part of the struct before the
> > extension was filled?
> > Does this guarantee really help?
> 
> yes, I think it helps. otherwise, it becomes a possible ABI issue.
> 
> >
> > >> I also wonder if the wording (elsewhere) of "returns EINVAL if size
> > >> is zero" is correct: Shouldn't it be "size must be >0"
> > >
> > > self_size is uint32_t, it is > 0 if not zero.
> >
> > The comment I saw is in another file (probably another patch), sorry
> > if this is confusing (I also find it confusing).
> > I will hopefully not be affected by the documentation, I just wanted
> > to point out that the wording is misleading imo, because structs do not
> return errors.
> 
> I see, it is in frame.h of patch 1/2.
> 
> imho, current wording is not misleading, the wording exactly matches the
> code, while the code is acceptable.
> There is also an explicit note (very close these wording) that the correct value
> should be sizeof(AVRegionOfInterest).
> 
> Regarding "structs do not return errors":
> 
> > or similar? A struct can hardly return an error, no?
> it is caller's responsibility to set the value to be sizeof(AVRegionOfInterest).
> There will be an error returned if the caller does not set it explicitly.
> 
> >
> > Carl Eugen
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list