[FFmpeg-devel] libavcodec/h264_sei: Don't log random user data.

Submitted by Kieran Kunhya on May 5, 2017, 9:30 p.m.

Details

Message ID CAK+ULv61-7S7iNzbABf79UEhTNJNwwBWKEow=a=PCEhzqJdGWQ@mail.gmail.com
State New
Headers show

Commit Message

Kieran Kunhya May 5, 2017, 9:30 p.m.
From d43c0366d4ba128772dc7909152f4e9635f269cb Mon Sep 17 00:00:00 2001
From: Kieran Kunhya <kierank@obe.tv>
Date: Fri, 5 May 2017 14:29:59 -0700
Subject: [PATCH] libavcodec/h264_sei: Don't log random user data. This
 prevents terminal junk.

---
 libavcodec/h264_sei.c | 3 ---
 1 file changed, 3 deletions(-)

     return 0;
 }

Comments

Michael Niedermayer May 6, 2017, 4:37 p.m.
On Fri, May 05, 2017 at 09:30:54PM +0000, Kieran Kunhya wrote:
> From d43c0366d4ba128772dc7909152f4e9635f269cb Mon Sep 17 00:00:00 2001
> From: Kieran Kunhya <kierank@obe.tv>
> Date: Fri, 5 May 2017 14:29:59 -0700
> Subject: [PATCH] libavcodec/h264_sei: Don't log random user data. This
>  prevents terminal junk.
> 
> ---
>  libavcodec/h264_sei.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
> index a7e627eba3..c81ddfef4f 100644
> --- a/libavcodec/h264_sei.c
> +++ b/libavcodec/h264_sei.c
> @@ -257,9 +257,6 @@ static int
> decode_unregistered_user_data(H264SEIUnregistered *h, GetBitContext *
>      if (e == 1 && build == 1 && !strncmp(user_data+16, "x264 - core 0000",
> 16))
>          h->x264_build = 67;
> 
> -    if (strlen(user_data + 16) > 0)
> -        av_log(logctx, AV_LOG_DEBUG, "user data:\"%s\"\n", user_data + 16);

This is just at debug level.
Do you have an example (file/usecase) where this causes excessive
output ?


[...]
James Almer May 6, 2017, 4:45 p.m.
On 5/6/2017 1:37 PM, Michael Niedermayer wrote:
> On Fri, May 05, 2017 at 09:30:54PM +0000, Kieran Kunhya wrote:
>> From d43c0366d4ba128772dc7909152f4e9635f269cb Mon Sep 17 00:00:00 2001
>> From: Kieran Kunhya <kierank@obe.tv>
>> Date: Fri, 5 May 2017 14:29:59 -0700
>> Subject: [PATCH] libavcodec/h264_sei: Don't log random user data. This
>>  prevents terminal junk.
>>
>> ---
>>  libavcodec/h264_sei.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
>> index a7e627eba3..c81ddfef4f 100644
>> --- a/libavcodec/h264_sei.c
>> +++ b/libavcodec/h264_sei.c
>> @@ -257,9 +257,6 @@ static int
>> decode_unregistered_user_data(H264SEIUnregistered *h, GetBitContext *
>>      if (e == 1 && build == 1 && !strncmp(user_data+16, "x264 - core 0000",
>> 16))
>>          h->x264_build = 67;
>>
>> -    if (strlen(user_data + 16) > 0)
>> -        av_log(logctx, AV_LOG_DEBUG, "user data:\"%s\"\n", user_data + 16);
> 
> This is just at debug level.
> Do you have an example (file/usecase) where this causes excessive
> output ?

It's not so much the amount to be printed as it's about not printing
random binary data. x264 is no the only encoder that writes this SEI,
and other encoders dump binary data in it. This can trash the terminal
displaying the log message.
The amount also matters, but that's solved by making sure
strlen(user_data + 16) is a sane value.

The proper change here would be to look at the uuid and print only those
known to store printable strings, and of course after making sure they
are in fact printable.
As of now, that means x264's uuid only.
Kieran Kunhya Nov. 4, 2017, 6:23 p.m.
On Sat, 6 May 2017 at 17:51 James Almer <jamrial@gmail.com> wrote:

> The proper change here would be to look at the uuid and print only those
> known to store printable strings, and of course after making sure they
> are in fact printable.
> As of now, that means x264's uuid only.
>

The UUID can be corrupt as well.
We shouldn't be printing random data from the bitstream to the users
terminal.

Kieran
Michael Niedermayer Nov. 4, 2017, 6:27 p.m.
On Sat, Nov 04, 2017 at 06:23:12PM +0000, Kieran Kunhya wrote:
> On Sat, 6 May 2017 at 17:51 James Almer <jamrial@gmail.com> wrote:
> 
> > The proper change here would be to look at the uuid and print only those
> > known to store printable strings, and of course after making sure they
> > are in fact printable.
> > As of now, that means x264's uuid only.
> >
> 
> The UUID can be corrupt as well.
> We shouldn't be printing random data from the bitstream to the users
> terminal.

i agree


[...]
Kieran Kunhya Nov. 4, 2017, 7:27 p.m.
On Sat, 4 Nov 2017 at 18:28 Michael Niedermayer <michael@niedermayer.cc>
wrote:

> > The UUID can be corrupt as well.
> > We shouldn't be printing random data from the bitstream to the users
> > terminal.
>
> i agree
>

Ok, I will push unless someone objects.

Kieran

Patch hide | download patch | download mbox

diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
index a7e627eba3..c81ddfef4f 100644
--- a/libavcodec/h264_sei.c
+++ b/libavcodec/h264_sei.c
@@ -257,9 +257,6 @@  static int
decode_unregistered_user_data(H264SEIUnregistered *h, GetBitContext *
     if (e == 1 && build == 1 && !strncmp(user_data+16, "x264 - core 0000",
16))
         h->x264_build = 67;

-    if (strlen(user_data + 16) > 0)
-        av_log(logctx, AV_LOG_DEBUG, "user data:\"%s\"\n", user_data + 16);
-
     av_free(user_data);