this patch set asks for pushing if no more concerns, thanks.
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf > Of Guo, Yejun > Sent: Friday, January 11, 2019 9:39 AM > To: FFmpeg development discussions and patches <ffmpeg- > de...@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-boun...@ffmpeg.org] On > Behalf > > Of Carl Eugen Hoyos > > Sent: Friday, January 11, 2019 8:54 AM > > To: FFmpeg development discussions and patches <ffmpeg- > > de...@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....@intel.com>: > > > > >> 2019-01-10 9:54 GMT+01:00, Guo, Yejun <yejun....@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@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
