avprobe: Handle invalid values for the color description

Message ID 20170417134854.4658-1-lu_zero@gentoo.org
State New
Headers show

Commit Message

Luca Barbato April 17, 2017, 1:48 p.m.
print_str() cannot print NULL.

Bug-Id: 1040
CC: libav-stable@libav.org
---
 avtools/avprobe.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

Comments

Vittorio Giovara April 17, 2017, 4:04 p.m. | #1
On Mon, Apr 17, 2017 at 9:48 AM, Luca Barbato <lu_zero@gentoo.org> wrote:
> print_str() cannot print NULL.
>
> Bug-Id: 1040
> CC: libav-stable@libav.org
> ---
>  avtools/avprobe.c | 36 +++++++++++++++++++++++++++++++-----
>  1 file changed, 31 insertions(+), 5 deletions(-)

I'm not sure about this. First of all these values are not invalid but
simply unknown.
Secondly I'm starting to think whether these name API should return a
simple "" instead of NULL.
Any opinions?
Luca Barbato April 18, 2017, 5:11 p.m. | #2
On 17/04/2017 18:04, Vittorio Giovara wrote:
> On Mon, Apr 17, 2017 at 9:48 AM, Luca Barbato <lu_zero@gentoo.org> wrote:
>> print_str() cannot print NULL.
>>
>> Bug-Id: 1040
>> CC: libav-stable@libav.org
>> ---
>>  avtools/avprobe.c | 36 +++++++++++++++++++++++++++++++-----
>>  1 file changed, 31 insertions(+), 5 deletions(-)
> 
> I'm not sure about this. First of all these values are not invalid but
> simply unknown.

So is it ok for you if I use Unknown (something) ?

> Secondly I'm starting to think whether these name API should return a
> simple "" instead of NULL.
> Any opinions?

I prefer NULL since it is easier to compare than ret[0], but I do not
have a strong opinion in this regard.

Just I'd rather have all the functions behave the same :)

lu
Luca Barbato April 23, 2017, 1:23 p.m. | #3
On 4/18/17 7:11 PM, Luca Barbato wrote:
> On 17/04/2017 18:04, Vittorio Giovara wrote:
>> On Mon, Apr 17, 2017 at 9:48 AM, Luca Barbato <lu_zero@gentoo.org> wrote:
>>> print_str() cannot print NULL.
>>>
>>> Bug-Id: 1040
>>> CC: libav-stable@libav.org
>>> ---
>>>  avtools/avprobe.c | 36 +++++++++++++++++++++++++++++++-----
>>>  1 file changed, 31 insertions(+), 5 deletions(-)
>>
>> I'm not sure about this. First of all these values are not invalid but
>> simply unknown.
> 
> So is it ok for you if I use Unknown (something) ?
> 

Ping on that, I'd push this and change it back if there consensus on how
those functions should behave tilts on having "unknown" returned.

lu
Vittorio Giovara April 24, 2017, 3:05 p.m. | #4
On Sun, Apr 23, 2017 at 9:23 AM, Luca Barbato <lu_zero@gentoo.org> wrote:
> On 4/18/17 7:11 PM, Luca Barbato wrote:
>> On 17/04/2017 18:04, Vittorio Giovara wrote:
>>> On Mon, Apr 17, 2017 at 9:48 AM, Luca Barbato <lu_zero@gentoo.org> wrote:
>>>> print_str() cannot print NULL.
>>>>
>>>> Bug-Id: 1040
>>>> CC: libav-stable@libav.org
>>>> ---
>>>>  avtools/avprobe.c | 36 +++++++++++++++++++++++++++++++-----
>>>>  1 file changed, 31 insertions(+), 5 deletions(-)
>>>
>>> I'm not sure about this. First of all these values are not invalid but
>>> simply unknown.
>>
>> So is it ok for you if I use Unknown (something) ?
>>
>
> Ping on that, I'd push this and change it back if there consensus on how
> those functions should behave tilts on having "unknown" returned.
>
> lu
>

go for it

Patch

diff --git a/avtools/avprobe.c b/avtools/avprobe.c
index 68f19220c8..92986b5693 100644
--- a/avtools/avprobe.c
+++ b/avtools/avprobe.c
@@ -609,6 +609,12 @@  static char *tag_string(char *buf, int buf_size, int tag)
     return buf;
 }
 
+static char *invalid_string(char *buf, int buf_size, int val)
+{
+    snprintf(buf, buf_size, "Invalid (%d)", val);
+    return buf;
+}
+
 static void show_packet(AVFormatContext *fmt_ctx, AVPacket *pkt)
 {
     char val_str[128];
@@ -660,6 +666,7 @@  static void show_stream(InputFile *ifile, InputStream *ist)
     char val_str[128];
     AVRational display_aspect_ratio, *sar = NULL;
     const AVPixFmtDescriptor *desc;
+    const char *val;
 
     probe_object_header("stream");
 
@@ -718,11 +725,30 @@  static void show_stream(InputFile *ifile, InputStream *ist)
         probe_str("pix_fmt", desc ? desc->name : "unknown");
         probe_int("level", par->level);
 
-        probe_str("color_range", av_color_range_name    (par->color_range));
-        probe_str("color_space", av_color_space_name    (par->color_space));
-        probe_str("color_trc",   av_color_transfer_name (par->color_trc));
-        probe_str("color_pri",   av_color_primaries_name(par->color_primaries));
-        probe_str("chroma_loc", av_chroma_location_name (par->chroma_location));
+        val = av_color_range_name(par->color_range);
+        if (!val)
+            val = invalid_string(val_str, sizeof(val_str), par->color_range);
+        probe_str("color_range", val);
+
+        val = av_color_space_name(par->color_space);
+        if (!val)
+            val = invalid_string(val_str, sizeof(val_str), par->color_space);
+        probe_str("color_space", val);
+
+        val = av_color_transfer_name(par->color_trc);
+        if (!val)
+            val = invalid_string(val_str, sizeof(val_str), par->color_trc);
+        probe_str("color_trc", val);
+
+        val = av_color_primaries_name(par->color_primaries);
+        if (!val)
+            val = invalid_string(val_str, sizeof(val_str), par->color_primaries);
+        probe_str("color_pri", val);
+
+        val = av_chroma_location_name(par->chroma_location);
+        if (!val)
+            val = invalid_string(val_str, sizeof(val_str), par->chroma_location);
+        probe_str("chroma_loc", val);
         break;
 
     case AVMEDIA_TYPE_AUDIO: