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

[Guo, Yejun]

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