[FFmpeg-devel] lavu/frame: Try to improve the documentation wording

Submitted by Carl Eugen Hoyos on Jan. 18, 2019, 11:38 a.m.

Details

Message ID CAB0OVGo6H0V+0z_nak11WqxO4C3sOydSU5JutkAJ17Yp=On8zA@mail.gmail.com
State New
Headers show

Commit Message

Carl Eugen Hoyos Jan. 18, 2019, 11:38 a.m.
Hi!

Attached patch tries to improve the ROI documentation wording, C
structs cannot return errors.

Please comment, Carl Eugen

Comments

Michael Niedermayer Jan. 19, 2019, 10:39 p.m.
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

[...]
Guo, Yejun Jan. 21, 2019, 2:06 a.m.
> -----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.
Carl Eugen Hoyos Jan. 21, 2019, 2:13 a.m.
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
Guo, Yejun Jan. 21, 2019, 2:38 a.m.
> -----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

Patch hide | download patch | download mbox

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