| Message ID | CAB0OVGo6H0V+0z_nak11WqxO4C3sOydSU5JutkAJ17Yp=On8zA@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Jan 18, 2019 at 12:38:20PM +0100, Carl Eugen Hoyos wrote: > Hi! > > Attached patch tries to improve the ROI documentation wording, C > structs cannot return errors. > > Please comment, Carl Eugen > frame.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > 888a2470d43113b08b0ef3ddd03bda528f873ccc 0001-lavu-frame-Try-to-improve-the-documentation-wording.patch > From 4abd545e7ab463c97bf816b270544eee25c4755b Mon Sep 17 00:00:00 2001 > From: Carl Eugen Hoyos <ceffmpeg@gmail.com> > Date: Fri, 18 Jan 2019 12:35:44 +0100 > Subject: [PATCH] lavu/frame: Try to improve the documentation wording. > > C structs cannot return errors. > --- > libavutil/frame.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavutil/frame.h b/libavutil/frame.h > index 8aa3e88..1990320 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -218,8 +218,8 @@ typedef struct AVFrameSideData { > * if the codec requires an alignment. > * If the regions overlap, the last value in the list will be used. > * > - * qoffset is quant offset, and base rule here: > - * returns EINVAL if AVRational.den is zero. > + * qoffset is quantization offset: > + * Encoders will return EINVAL if qoffset.den is zero. > * the value (num/den) range is [-1.0, 1.0], clamp to +-1.0 if out of range. > * 0 means no picture quality change, > * negative offset asks for better quality (and the best with value -1.0), The field specific documentation should be added to each of the fields using correct doxygen syntax. About EINVAL, i would tend to document what is valid and what is not instead of documenting what some code will do with invalid values. The user has no reason to ever use invalid values so that information should have no value. And just has some chance to be incorrect now or later Also the wording of the unchanged parts sound funky, some native english speaker should check and reword more of this i think. thanks [...]
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Michael Niedermayer > Sent: Sunday, January 20, 2019 6:39 AM > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH]lavu/frame: Try to improve the > documentation wording > > On Fri, Jan 18, 2019 at 12:38:20PM +0100, Carl Eugen Hoyos wrote: > > Hi! > > > > Attached patch tries to improve the ROI documentation wording, C > > structs cannot return errors. > > > > Please comment, Carl Eugen > > > frame.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > 888a2470d43113b08b0ef3ddd03bda528f873ccc > > 0001-lavu-frame-Try-to-improve-the-documentation-wording.patch > > From 4abd545e7ab463c97bf816b270544eee25c4755b Mon Sep 17 00:00:00 > 2001 > > From: Carl Eugen Hoyos <ceffmpeg@gmail.com> > > Date: Fri, 18 Jan 2019 12:35:44 +0100 > > Subject: [PATCH] lavu/frame: Try to improve the documentation wording. > > > > C structs cannot return errors. > > --- > > libavutil/frame.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libavutil/frame.h b/libavutil/frame.h index > > 8aa3e88..1990320 100644 > > --- a/libavutil/frame.h > > +++ b/libavutil/frame.h > > @@ -218,8 +218,8 @@ typedef struct AVFrameSideData { > > * if the codec requires an alignment. > > * If the regions overlap, the last value in the list will be used. > > * > > - * qoffset is quant offset, and base rule here: > > - * returns EINVAL if AVRational.den is zero. > > + * qoffset is quantization offset: > > + * Encoders will return EINVAL if qoffset.den is zero. > > * the value (num/den) range is [-1.0, 1.0], clamp to +-1.0 if out of range. > > * 0 means no picture quality change, > > * negative offset asks for better quality (and the best with value > > -1.0), > > The field specific documentation should be added to each of the fields using > correct doxygen syntax. > > About EINVAL, i would tend to document what is valid and what is not > instead of documenting what some code will do with invalid values. > The user has no reason to ever use invalid values so that information should > have no value. And just has some chance to be incorrect now or later > > Also the wording of the unchanged parts sound funky, some native english > speaker should check and reword more of this i think. Sorry for the wording, hope it would be better. For the struct field doxygen syntax, I got the comment to be 'above the typedef', I probably mis-understood it. Will study doxygen syntax and be more careful about it in later patches. And don't plan to change this one since rewording is needed. > > thanks > > [...] > > > -- > Michael GnuPG fingerprint: > 9FF2128B147EF6730BADF133611EC787040B0FAB > > If you drop bombs on a foreign country and kill a hundred thousand innocent > people, expect your government to call the consequence "unprovoked > inhuman terrorist attacks" and use it to justify dropping more bombs and > killing more people. The technology changed, the idea is old.
2019-01-21 3:06 GMT+01:00, Guo, Yejun <yejun.guo@intel.com>: > > >> -----Original Message----- >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf >> Of Michael Niedermayer >> Sent: Sunday, January 20, 2019 6:39 AM >> To: FFmpeg development discussions and patches <ffmpeg- >> devel@ffmpeg.org> >> Subject: Re: [FFmpeg-devel] [PATCH]lavu/frame: Try to improve the >> documentation wording >> >> On Fri, Jan 18, 2019 at 12:38:20PM +0100, Carl Eugen Hoyos wrote: >> > Hi! >> > >> > Attached patch tries to improve the ROI documentation wording, C >> > structs cannot return errors. >> > >> > Please comment, Carl Eugen >> >> > frame.h | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > 888a2470d43113b08b0ef3ddd03bda528f873ccc >> > 0001-lavu-frame-Try-to-improve-the-documentation-wording.patch >> > From 4abd545e7ab463c97bf816b270544eee25c4755b Mon Sep 17 00:00:00 >> 2001 >> > From: Carl Eugen Hoyos <ceffmpeg@gmail.com> >> > Date: Fri, 18 Jan 2019 12:35:44 +0100 >> > Subject: [PATCH] lavu/frame: Try to improve the documentation wording. >> > >> > C structs cannot return errors. >> > --- >> > libavutil/frame.h | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/libavutil/frame.h b/libavutil/frame.h index >> > 8aa3e88..1990320 100644 >> > --- a/libavutil/frame.h >> > +++ b/libavutil/frame.h >> > @@ -218,8 +218,8 @@ typedef struct AVFrameSideData { >> > * if the codec requires an alignment. >> > * If the regions overlap, the last value in the list will be used. >> > * >> > - * qoffset is quant offset, and base rule here: >> > - * returns EINVAL if AVRational.den is zero. >> > + * qoffset is quantization offset: >> > + * Encoders will return EINVAL if qoffset.den is zero. >> > * the value (num/den) range is [-1.0, 1.0], clamp to +-1.0 if out of >> > range. >> > * 0 means no picture quality change, >> > * negative offset asks for better quality (and the best with value >> > -1.0), >> >> The field specific documentation should be added to each of the fields >> using >> correct doxygen syntax. >> >> About EINVAL, i would tend to document what is valid and what is not >> instead of documenting what some code will do with invalid values. >> The user has no reason to ever use invalid values so that information >> should >> have no value. And just has some chance to be incorrect now or later >> >> Also the wording of the unchanged parts sound funky, some native english >> speaker should check and reword more of this i think. > > Sorry for the wording Didn't I ask you to improve it? Please send a patch, Carl Eugen
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Carl Eugen Hoyos > Sent: Monday, January 21, 2019 10:13 AM > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH]lavu/frame: Try to improve the > documentation wording > > 2019-01-21 3:06 GMT+01:00, Guo, Yejun <yejun.guo@intel.com>: > > > > > >> -----Original Message----- > >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On > Behalf > >> Of Michael Niedermayer > >> Sent: Sunday, January 20, 2019 6:39 AM > >> To: FFmpeg development discussions and patches <ffmpeg- > >> devel@ffmpeg.org> > >> Subject: Re: [FFmpeg-devel] [PATCH]lavu/frame: Try to improve the > >> documentation wording > >> > >> On Fri, Jan 18, 2019 at 12:38:20PM +0100, Carl Eugen Hoyos wrote: > >> > Hi! > >> > > >> > Attached patch tries to improve the ROI documentation wording, C > >> > structs cannot return errors. > >> > > >> > Please comment, Carl Eugen > >> > >> > frame.h | 4 ++-- > >> > 1 file changed, 2 insertions(+), 2 deletions(-) > >> > 888a2470d43113b08b0ef3ddd03bda528f873ccc > >> > 0001-lavu-frame-Try-to-improve-the-documentation-wording.patch > >> > From 4abd545e7ab463c97bf816b270544eee25c4755b Mon Sep 17 > 00:00:00 > >> 2001 > >> > From: Carl Eugen Hoyos <ceffmpeg@gmail.com> > >> > Date: Fri, 18 Jan 2019 12:35:44 +0100 > >> > Subject: [PATCH] lavu/frame: Try to improve the documentation > wording. > >> > > >> > C structs cannot return errors. > >> > --- > >> > libavutil/frame.h | 4 ++-- > >> > 1 file changed, 2 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/libavutil/frame.h b/libavutil/frame.h index > >> > 8aa3e88..1990320 100644 > >> > --- a/libavutil/frame.h > >> > +++ b/libavutil/frame.h > >> > @@ -218,8 +218,8 @@ typedef struct AVFrameSideData { > >> > * if the codec requires an alignment. > >> > * If the regions overlap, the last value in the list will be used. > >> > * > >> > - * qoffset is quant offset, and base rule here: > >> > - * returns EINVAL if AVRational.den is zero. > >> > + * qoffset is quantization offset: > >> > + * Encoders will return EINVAL if qoffset.den is zero. > >> > * the value (num/den) range is [-1.0, 1.0], clamp to +-1.0 if out > >> > of range. > >> > * 0 means no picture quality change, > >> > * negative offset asks for better quality (and the best with > >> > value -1.0), > >> > >> The field specific documentation should be added to each of the > >> fields using correct doxygen syntax. > >> > >> About EINVAL, i would tend to document what is valid and what is not > >> instead of documenting what some code will do with invalid values. > >> The user has no reason to ever use invalid values so that information > >> should have no value. And just has some chance to be incorrect now or > >> later > >> > >> Also the wording of the unchanged parts sound funky, some native > >> english speaker should check and reword more of this i think. > > > > Sorry for the wording > > Didn't I ask you to improve it? I got your point only after seeing this patch. And your patch is better. > > Please send a patch, Carl Eugen For the unchanged funky parts, I did try hard to make the description clear, but can hardly improve the wording as a native speaker in a short time. Anyway, before see an improvement patch, I'll keep it in my mind and improve it once I get new ideas for the rewording. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
From 4abd545e7ab463c97bf816b270544eee25c4755b Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceffmpeg@gmail.com> Date: Fri, 18 Jan 2019 12:35:44 +0100 Subject: [PATCH] lavu/frame: Try to improve the documentation wording. C structs cannot return errors. --- libavutil/frame.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libavutil/frame.h b/libavutil/frame.h index 8aa3e88..1990320 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -218,8 +218,8 @@ typedef struct AVFrameSideData { * if the codec requires an alignment. * If the regions overlap, the last value in the list will be used. * - * qoffset is quant offset, and base rule here: - * returns EINVAL if AVRational.den is zero. + * qoffset is quantization offset: + * Encoders will return EINVAL if qoffset.den is zero. * the value (num/den) range is [-1.0, 1.0], clamp to +-1.0 if out of range. * 0 means no picture quality change, * negative offset asks for better quality (and the best with value -1.0), -- 1.7.10.4