diff mbox

[FFmpeg-devel,01/10] lavu: add av_fourcc_make_string() and av_4cc2str()

Message ID 20170328081946.GN3964@golem.pkh.me
State Superseded
Headers show

Commit Message

Clément Bœsch March 28, 2017, 8:19 a.m. UTC 
On Tue, Mar 28, 2017 at 12:47:39AM +0200, Alexander Strasser wrote:
[...]
> > +#define AV_FOURCC_MAX_STRING_SIZE 32
> 
> Should be fine, though I don't know how you came up with this max size in particular.
> 

IIRC the actual maximum is 21 characters, I just took the closest superior
power of two. Probably just like we did for AV_TS_MAX_STRING_SIZE.

> > +
> > +#define av_4cc2str(fourcc) av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
> 
> Did your write it as 4cc2str to make the name shorter?
> I guess fourcc2str is more readable and more consistent.
> 

This is a shorthand, so yeah I wanted to keep it short. If you replace
"4cc" with "fourcc", why not do that for the '2' as well? Writing "four"
but keeping '2' was kind of weird to me :)

> > +
> > +/**
> > + * Fill the provided buffer with a string containing a FourCC (four-character
> > + * code) representation.
> > + *
> > + * @param buf    a buffer with size in bytes of at least AV_FOURCC_MAX_STRING_SIZE
> > + * @param fourcc the fourcc to represent
> > + * @return the buffer in input
> > + */
> > +char *av_fourcc_make_string(char *buf, uint32_t fourcc);
> > +
> >  /**
> >   * @}
> >   * @}
> > diff --git a/libavutil/utils.c b/libavutil/utils.c
> > index 36e4dd5fdb..ba557b9252 100644
> > --- a/libavutil/utils.c
> > +++ b/libavutil/utils.c
> > @@ -121,6 +121,27 @@ unsigned av_int_list_length_for_size(unsigned elsize,
> >      return i;
> >  }
> >  
> > +char *av_fourcc_make_string(char *buf, uint32_t fourcc)
> > +{
> > +    int i, len;
> > +    char *orig_buf = buf;
> > +    size_t buf_size = AV_FOURCC_MAX_STRING_SIZE;
> > +
> > +#define TAG_PRINT(x)                                              \
> > +    (((x) >= '0' && (x) <= '9') ||                                \
> > +     ((x) >= 'a' && (x) <= 'z') || ((x) >= 'A' && (x) <= 'Z') ||  \
> > +     ((x) == '.' || (x) == ' ' || (x) == '-' || (x) == '_'))
> 
> You could spare this macro, by using a temporary char for the character and
> an int to indicate if it is printable in the loop below. Would probably be 
> 1 or 2 lines longer.
> 
> You might want to at least rename TAG_PRINT to IS_PRINTABLE or
> IS_PRINTABLE_FOURCC_CHAR or similar as TAG_PRINT is a rather confusing name.
> 
> Also if the macro is only introduced for use this function, #undef could be 
> used, but I don't think we really do it anywhere else.
> 

Yeah well I just took the existing code in libavcodec, so I didn't want to
change it too much.

But you're right on all your comments so I just adjusted the function. See
patch attached.

[...]

Thanks

Comments

Matthias Hunstock March 28, 2017, 10:57 a.m. UTC | #1 
Am 28.03.2017 um 10:19 schrieb Clément Bœsch:
> But you're right on all your comments so I just adjusted the function. See
> patch attached.

Did you also update the deprecation notice in [PATCH 02/10]?

Matthias
Clément Bœsch March 28, 2017, 10:59 a.m. UTC | #2 
On Tue, Mar 28, 2017 at 12:57:52PM +0200, Matthias Hunstock wrote:
> Am 28.03.2017 um 10:19 schrieb Clément Bœsch:
> > But you're right on all your comments so I just adjusted the function. See
> > patch attached.
> 
> Did you also update the deprecation notice in [PATCH 02/10]?
> 

I didn't rename the prototype or the macro so it doesn't need any update,
or am I missing something?
Michael Niedermayer March 28, 2017, 4:12 p.m. UTC | #3 
On Tue, Mar 28, 2017 at 10:19:46AM +0200, Clément Bœsch wrote:
[...]
> diff --git a/libavutil/utils.c b/libavutil/utils.c
> index 36e4dd5fdb..29f2746338 100644
> --- a/libavutil/utils.c
> +++ b/libavutil/utils.c
> @@ -121,6 +121,29 @@ unsigned av_int_list_length_for_size(unsigned elsize,
>      return i;
>  }
>  
> +char *av_fourcc_make_string(char *buf, uint32_t fourcc)
> +{
> +    int i;
> +    char *orig_buf = buf;
> +    size_t buf_size = AV_FOURCC_MAX_STRING_SIZE;
> +
> +    for (i = 0; i < 4; i++) {
> +        const char c = fourcc & 0xff;
> +        const int print_chr = (c >= '0' && c <= '9') ||
> +                              (c >= 'a' && c <= 'z') ||
> +                              (c >= 'A' && c <= 'Z') ||
> +                              (c && strchr(". -_", c));
> +        const int len = snprintf(buf, buf_size, print_chr ? "%c" : "[%d]", c);

this prints values over 127 as negative if char is signed

[...]
Clément Bœsch March 28, 2017, 4:16 p.m. UTC | #4 
On Tue, Mar 28, 2017 at 06:12:14PM +0200, Michael Niedermayer wrote:
> On Tue, Mar 28, 2017 at 10:19:46AM +0200, Clément Bœsch wrote:
> [...]
> > diff --git a/libavutil/utils.c b/libavutil/utils.c
> > index 36e4dd5fdb..29f2746338 100644
> > --- a/libavutil/utils.c
> > +++ b/libavutil/utils.c
> > @@ -121,6 +121,29 @@ unsigned av_int_list_length_for_size(unsigned elsize,
> >      return i;
> >  }
> >  
> > +char *av_fourcc_make_string(char *buf, uint32_t fourcc)
> > +{
> > +    int i;
> > +    char *orig_buf = buf;
> > +    size_t buf_size = AV_FOURCC_MAX_STRING_SIZE;
> > +
> > +    for (i = 0; i < 4; i++) {
> > +        const char c = fourcc & 0xff;
> > +        const int print_chr = (c >= '0' && c <= '9') ||
> > +                              (c >= 'a' && c <= 'z') ||
> > +                              (c >= 'A' && c <= 'Z') ||
> > +                              (c && strchr(". -_", c));
> > +        const int len = snprintf(buf, buf_size, print_chr ? "%c" : "[%d]", c);
> 
> this prints values over 127 as negative if char is signed
> 

oh i thought i changed that to const int c = ... 
forgot to resend the patch, consider it fixed locally
Alexander Strasser March 28, 2017, 8:24 p.m. UTC | #5 
On 2017-03-28 10:19 +0200, Clément Bœsch wrote:
> On Tue, Mar 28, 2017 at 12:47:39AM +0200, Alexander Strasser wrote:
> [...]
> > > +#define AV_FOURCC_MAX_STRING_SIZE 32
> > 
> > Should be fine, though I don't know how you came up with this max size in particular.
> > 
> 
> IIRC the actual maximum is 21 characters, I just took the closest superior
> power of two. Probably just like we did for AV_TS_MAX_STRING_SIZE.

OK

> > > +
> > > +#define av_4cc2str(fourcc) av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
> > 
> > Did your write it as 4cc2str to make the name shorter?
> > I guess fourcc2str is more readable and more consistent.
> > 
> 
> This is a shorthand, so yeah I wanted to keep it short. If you replace
> "4cc" with "fourcc", why not do that for the '2' as well? Writing "four"
> but keeping '2' was kind of weird to me :)

Well, to expand on my point a bit. The 2 in these kind of functions works
like a delimiter to me e.g. "a2b"; easy to tokenize. When looking at "4a2b",
it is a bit harder. I know the example is a bit more extreme.

The other thing is that 4 is also sometimes used as meaning "for", similar 
to the usage of 2 as meaning "to".


TL;DR: I am fine with the name you chose, just wanted to tell my impression.

Regarding the follow-up patches renaming would also cause a fair bit of labour
to you. So I can totally understand if you keep the name.


> > > +
> > > +/**
> > > + * Fill the provided buffer with a string containing a FourCC (four-character
> > > + * code) representation.
> > > + *
> > > + * @param buf    a buffer with size in bytes of at least AV_FOURCC_MAX_STRING_SIZE
> > > + * @param fourcc the fourcc to represent
> > > + * @return the buffer in input
> > > + */
> > > +char *av_fourcc_make_string(char *buf, uint32_t fourcc);
> > > +
> > >  /**
> > >   * @}
> > >   * @}
> > > diff --git a/libavutil/utils.c b/libavutil/utils.c
> > > index 36e4dd5fdb..ba557b9252 100644
> > > --- a/libavutil/utils.c
> > > +++ b/libavutil/utils.c
> > > @@ -121,6 +121,27 @@ unsigned av_int_list_length_for_size(unsigned elsize,
> > >      return i;
> > >  }
> > >  
> > > +char *av_fourcc_make_string(char *buf, uint32_t fourcc)
> > > +{
> > > +    int i, len;
> > > +    char *orig_buf = buf;
> > > +    size_t buf_size = AV_FOURCC_MAX_STRING_SIZE;
> > > +
> > > +#define TAG_PRINT(x)                                              \
> > > +    (((x) >= '0' && (x) <= '9') ||                                \
> > > +     ((x) >= 'a' && (x) <= 'z') || ((x) >= 'A' && (x) <= 'Z') ||  \
> > > +     ((x) == '.' || (x) == ' ' || (x) == '-' || (x) == '_'))
> > 
> > You could spare this macro, by using a temporary char for the character and
> > an int to indicate if it is printable in the loop below. Would probably be 
> > 1 or 2 lines longer.
> > 
> > You might want to at least rename TAG_PRINT to IS_PRINTABLE or
> > IS_PRINTABLE_FOURCC_CHAR or similar as TAG_PRINT is a rather confusing name.
> > 
> > Also if the macro is only introduced for use this function, #undef could be 
> > used, but I don't think we really do it anywhere else.
> > 
> 
> Yeah well I just took the existing code in libavcodec, so I didn't want to
> change it too much.
> 
> But you're right on all your comments so I just adjusted the function. See
> patch attached.

I will comment on the patch in response to Michael's comment.


  Alexander
Alexander Strasser March 28, 2017, 8:31 p.m. UTC | #6 
On 2017-03-28 18:16 +0200, Clément Bœsch wrote:
> On Tue, Mar 28, 2017 at 06:12:14PM +0200, Michael Niedermayer wrote:
> > On Tue, Mar 28, 2017 at 10:19:46AM +0200, Clément Bœsch wrote:
> > [...]
> > > diff --git a/libavutil/utils.c b/libavutil/utils.c
> > > index 36e4dd5fdb..29f2746338 100644
> > > --- a/libavutil/utils.c
> > > +++ b/libavutil/utils.c
> > > @@ -121,6 +121,29 @@ unsigned av_int_list_length_for_size(unsigned elsize,
> > >      return i;
> > >  }
> > >  
> > > +char *av_fourcc_make_string(char *buf, uint32_t fourcc)
> > > +{
> > > +    int i;
> > > +    char *orig_buf = buf;
> > > +    size_t buf_size = AV_FOURCC_MAX_STRING_SIZE;
> > > +
> > > +    for (i = 0; i < 4; i++) {
> > > +        const char c = fourcc & 0xff;
> > > +        const int print_chr = (c >= '0' && c <= '9') ||
> > > +                              (c >= 'a' && c <= 'z') ||
> > > +                              (c >= 'A' && c <= 'Z') ||
> > > +                              (c && strchr(". -_", c));
> > > +        const int len = snprintf(buf, buf_size, print_chr ? "%c" : "[%d]", c);
> > 
> > this prints values over 127 as negative if char is signed
> > 
> 
> oh i thought i changed that to const int c = ... 
> forgot to resend the patch, consider it fixed locally

  Sorry, I probably hinted you in the wrong direction there. It came to my
mind while brushing my teeth yesterday. Using int for the var holding the 
byte value should be sufficient and especially avoid the unwanted sign
extension.


  LGTM with the fix you mentioned; thank you for making the adjustments!


  Alexander
Matthias Hunstock March 28, 2017, 9:02 p.m. UTC | #7 
Am 28.03.2017 um 12:59 schrieb Clément Bœsch:
> I didn't rename the prototype or the macro so it doesn't need any update,
> or am I missing something?

No you don't.. I did - sorry!

Matthias
Tobias Rapp March 29, 2017, 6:30 a.m. UTC | #8 
On 28.03.2017 10:19, Clément Bœsch wrote:
> On Tue, Mar 28, 2017 at 12:47:39AM +0200, Alexander Strasser wrote:
> [...]
>>> +
>>> +#define av_4cc2str(fourcc) av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
>>
>> Did your write it as 4cc2str to make the name shorter?
>> I guess fourcc2str is more readable and more consistent.
>>
>
> This is a shorthand, so yeah I wanted to keep it short. If you replace
> "4cc" with "fourcc", why not do that for the '2' as well? Writing "four"
> but keeping '2' was kind of weird to me :)

When grep'ing the source files it seems there are other "...2str" 
functions like av_ts2str and av_err2str. On the other hand the codec and 
(RIFF) chunk ids functions and variables use "fourcc" in 95% of the 
existing cases. Just my 2¢.

Regards,
Tobias
diff mbox

Patch

From 067d474907ed6c3170f2d1d05e5f9931648ce9b7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <u@pkh.me>
Date: Mon, 27 Mar 2017 01:05:18 +0200
Subject: [PATCH 01/15] lavu: add av_fourcc_make_string() and av_4cc2str()

---
 doc/APIchanges      |  4 ++++
 libavutil/avutil.h  | 14 ++++++++++++++
 libavutil/utils.c   | 23 +++++++++++++++++++++++
 libavutil/version.h |  2 +-
 4 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/doc/APIchanges b/doc/APIchanges
index 2274543024..ee062d7aeb 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,10 @@  libavutil:     2015-08-28
 
 API changes, most recent first:
 
+2017-03-xx - xxxxxxx - lavu 55.52.100 - avutil.h
+  add av_fourcc_make_string() function and av_4cc2str() macro to replace
+  av_get_codec_tag_string() from lavc.
+
 2017-03-xx - xxxxxxx - lavf 57.68.100 - avformat.h
   Deprecate that demuxers export the stream rotation angle in AVStream.metadata
   (via an entry named "rotate"). Use av_stream_get_side_data() with
diff --git a/libavutil/avutil.h b/libavutil/avutil.h
index e9aaa03722..98100fdcc5 100644
--- a/libavutil/avutil.h
+++ b/libavutil/avutil.h
@@ -343,6 +343,20 @@  FILE *av_fopen_utf8(const char *path, const char *mode);
  */
 AVRational av_get_time_base_q(void);
 
+#define AV_FOURCC_MAX_STRING_SIZE 32
+
+#define av_4cc2str(fourcc) av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
+
+/**
+ * Fill the provided buffer with a string containing a FourCC (four-character
+ * code) representation.
+ *
+ * @param buf    a buffer with size in bytes of at least AV_FOURCC_MAX_STRING_SIZE
+ * @param fourcc the fourcc to represent
+ * @return the buffer in input
+ */
+char *av_fourcc_make_string(char *buf, uint32_t fourcc);
+
 /**
  * @}
  * @}
diff --git a/libavutil/utils.c b/libavutil/utils.c
index 36e4dd5fdb..29f2746338 100644
--- a/libavutil/utils.c
+++ b/libavutil/utils.c
@@ -121,6 +121,29 @@  unsigned av_int_list_length_for_size(unsigned elsize,
     return i;
 }
 
+char *av_fourcc_make_string(char *buf, uint32_t fourcc)
+{
+    int i;
+    char *orig_buf = buf;
+    size_t buf_size = AV_FOURCC_MAX_STRING_SIZE;
+
+    for (i = 0; i < 4; i++) {
+        const char c = fourcc & 0xff;
+        const int print_chr = (c >= '0' && c <= '9') ||
+                              (c >= 'a' && c <= 'z') ||
+                              (c >= 'A' && c <= 'Z') ||
+                              (c && strchr(". -_", c));
+        const int len = snprintf(buf, buf_size, print_chr ? "%c" : "[%d]", c);
+        if (len < 0)
+            break;
+        buf += len;
+        buf_size = buf_size > len ? buf_size - len : 0;
+        fourcc >>= 8;
+    }
+
+    return orig_buf;
+}
+
 AVRational av_get_time_base_q(void)
 {
     return (AVRational){1, AV_TIME_BASE};
diff --git a/libavutil/version.h b/libavutil/version.h
index 3c86c26638..d6d78e7dc2 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@ 
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  55
-#define LIBAVUTIL_VERSION_MINOR  51
+#define LIBAVUTIL_VERSION_MINOR  52
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
-- 
2.12.0