Re: [FFmpeg-devel] [PATCH v11 3/4] avfilter/vf_showinfo: display H.26[45] user data unregistered sei message
On Thu, Jun 11, 2020 at 05:44:45PM +0200, Andreas Rheinhardt wrote:
> lance.lmw...@gmail.com:
> > On Wed, Jun 10, 2020 at 08:13:59PM +0200, Andreas Rheinhardt wrote:
> >> lance.lmw...@gmail.com:
> >>> From: Limin Wang
> >>>
> >>> Signed-off-by: Limin Wang
> >>> ---
> >>> libavfilter/vf_showinfo.c | 37 +
> >>> 1 file changed, 37 insertions(+)
> >>>
> >>> diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c
> >>> index 5d4aee4..2511da5 100644
> >>> --- a/libavfilter/vf_showinfo.c
> >>> +++ b/libavfilter/vf_showinfo.c
> >>> @@ -37,6 +37,7 @@
> >>> #include "libavutil/timecode.h"
> >>> #include "libavutil/mastering_display_metadata.h"
> >>> #include "libavutil/video_enc_params.h"
> >>> +#include "libavutil/avstring.h"
> >>>
> >>> #include "avfilter.h"
> >>> #include "internal.h"
> >>> @@ -190,6 +191,39 @@ static void dump_video_enc_params(AVFilterContext
> >>> *ctx, AVFrameSideData *sd)
> >>> av_log(ctx, AV_LOG_INFO, "%u blocks; ", par->nb_blocks);
> >>> }
> >>>
> >>> +static int string_is_print(const uint8_t *str)
> >>> +{
> >>> +while (av_isgraph(*str)) str++;
> >>> +return !*str;
> >>
> >> This is dangerous: The SEI message needn't be zero-terminated, so this
> >> may segfault. Furthermore, it is also wrong: If the user data payload
> >> happens to contain a binary zero preceded by printable ASCII characters,
> >> it will be considered a string, even if there are unprintable characters
> >> after the binary zero (which may really happen if the payload is
> >> actually binary).
> >
> > Give below test example, if a binary zero preceded by "hello", the %s will
> > print
> > out "hello" and ignore the other unprintable characters. I think it's
> > expected.
> > We only dump the printable result if it is string or parial string.
> >
> > unsigned char test[10];
> >
> > test[0] = 'h';
> > test[1] = 'e';
> > test[2] = 'l';
> > test[3] = 'l';
> > test[4] = 'o';
> > test[5] = 0;
> > test[6] = 133;
> > test[7] = 144;
> >
> > printf( "test: %s \n", test);
> >
> > So I thbink it's safety after add one padding zero bytes for the user data
> > buffer.
> >
>
> And who says that what is before the first binary zero is actually a
> string and not some actually binary data that just happens to be in the
> printable ASCII range and will be gibberish when printed?
Sorry, it seems that I misunderstand your comments at first, I have change to
check
whether *str is 0, please help to check whether my update patch solve your
question.
>
> >>
> >>> +}
> >>> +
> >>> +static void dump_sei_unregistered_metadata(AVFilterContext *ctx,
> >>> AVFrameSideData *sd)
> >>> +{
> >>> +const int uuid_size = 16;
> >>> +uint8_t *user_data = sd->data;
> >>> +
> >>> +if (sd->size < uuid_size) {
> >>> +av_log(ctx, AV_LOG_ERROR, "invalid data(%d < UUID(%d-bytes))",
> >>> sd->size, uuid_size);
> >>> +return;
> >>> +}
> >>> +
> >>> +av_log(ctx, AV_LOG_INFO, "User Data Unregistered:\n");
> >>> +av_log(ctx, AV_LOG_INFO, "UUID=");
> >>> +for (int i = 0; i < uuid_size; i++) {
> >>> +av_log(ctx, AV_LOG_INFO, "%02x", user_data[i]);
> >>> +if (i == 3 || i == 5 || i == 7 || i == 9)
> >>> +av_log(ctx, AV_LOG_INFO, "-");
> >>> +}
> >>> +av_log(ctx, AV_LOG_INFO, "\n");
> >>> +
> >>> +user_data += uuid_size;
> >>> +/* Only print the user data details if it's string */
> >>> +if (string_is_print(user_data)) {
> >>> +av_log(ctx, AV_LOG_INFO, "User Data=");
> >>> +av_log(ctx, AV_LOG_INFO, "%s", user_data);
> >>
> >> Given that the user_data needn't be zero-terminated, you must specify
> >> the precision (i.e. the number of bytes to write) here.
> >>
> >>> +}
> >>> +}
> >>> +
> >>> static void dump_color_property(AVFilterContext *ctx, AVFrame *frame)
> >>> {
> >>> const char *color_range_str =
> >>> av_color_range_name(frame->color_range);
> >>> @@ -375,6 +409,9 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> >>> *frame)
> >>> case AV_FRAME_DATA_VIDEO_ENC_PARAMS:
> >>> dump_video_enc_params(ctx, sd);
> >>> break;
> >>> +case AV_FRAME_DATA_SEI_UNREGISTERED:
> >>> +dump_sei_unregistered_metadata(ctx, sd);
> >>> +break;
> >>> default:
> >>> av_log(ctx, AV_LOG_WARNING, "unknown side data type %d (%d
> >>> bytes)",
> >>> sd->type, sd->size);
> >>>
> >>
> >> ___
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel@ffmpeg.org
> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >> To unsubscribe, visit link above, or email
> >> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
> >
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
>
Re: [FFmpeg-devel] [PATCH v11 3/4] avfilter/vf_showinfo: display H.26[45] user data unregistered sei message
lance.lmw...@gmail.com:
> On Wed, Jun 10, 2020 at 08:13:59PM +0200, Andreas Rheinhardt wrote:
>> lance.lmw...@gmail.com:
>>> From: Limin Wang
>>>
>>> Signed-off-by: Limin Wang
>>> ---
>>> libavfilter/vf_showinfo.c | 37 +
>>> 1 file changed, 37 insertions(+)
>>>
>>> diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c
>>> index 5d4aee4..2511da5 100644
>>> --- a/libavfilter/vf_showinfo.c
>>> +++ b/libavfilter/vf_showinfo.c
>>> @@ -37,6 +37,7 @@
>>> #include "libavutil/timecode.h"
>>> #include "libavutil/mastering_display_metadata.h"
>>> #include "libavutil/video_enc_params.h"
>>> +#include "libavutil/avstring.h"
>>>
>>> #include "avfilter.h"
>>> #include "internal.h"
>>> @@ -190,6 +191,39 @@ static void dump_video_enc_params(AVFilterContext
>>> *ctx, AVFrameSideData *sd)
>>> av_log(ctx, AV_LOG_INFO, "%u blocks; ", par->nb_blocks);
>>> }
>>>
>>> +static int string_is_print(const uint8_t *str)
>>> +{
>>> +while (av_isgraph(*str)) str++;
>>> +return !*str;
>>
>> This is dangerous: The SEI message needn't be zero-terminated, so this
>> may segfault. Furthermore, it is also wrong: If the user data payload
>> happens to contain a binary zero preceded by printable ASCII characters,
>> it will be considered a string, even if there are unprintable characters
>> after the binary zero (which may really happen if the payload is
>> actually binary).
>
> Give below test example, if a binary zero preceded by "hello", the %s will
> print
> out "hello" and ignore the other unprintable characters. I think it's
> expected.
> We only dump the printable result if it is string or parial string.
>
> unsigned char test[10];
>
> test[0] = 'h';
> test[1] = 'e';
> test[2] = 'l';
> test[3] = 'l';
> test[4] = 'o';
> test[5] = 0;
> test[6] = 133;
> test[7] = 144;
>
> printf( "test: %s \n", test);
>
> So I thbink it's safety after add one padding zero bytes for the user data
> buffer.
>
And who says that what is before the first binary zero is actually a
string and not some actually binary data that just happens to be in the
printable ASCII range and will be gibberish when printed?
>>
>>> +}
>>> +
>>> +static void dump_sei_unregistered_metadata(AVFilterContext *ctx,
>>> AVFrameSideData *sd)
>>> +{
>>> +const int uuid_size = 16;
>>> +uint8_t *user_data = sd->data;
>>> +
>>> +if (sd->size < uuid_size) {
>>> +av_log(ctx, AV_LOG_ERROR, "invalid data(%d < UUID(%d-bytes))",
>>> sd->size, uuid_size);
>>> +return;
>>> +}
>>> +
>>> +av_log(ctx, AV_LOG_INFO, "User Data Unregistered:\n");
>>> +av_log(ctx, AV_LOG_INFO, "UUID=");
>>> +for (int i = 0; i < uuid_size; i++) {
>>> +av_log(ctx, AV_LOG_INFO, "%02x", user_data[i]);
>>> +if (i == 3 || i == 5 || i == 7 || i == 9)
>>> +av_log(ctx, AV_LOG_INFO, "-");
>>> +}
>>> +av_log(ctx, AV_LOG_INFO, "\n");
>>> +
>>> +user_data += uuid_size;
>>> +/* Only print the user data details if it's string */
>>> +if (string_is_print(user_data)) {
>>> +av_log(ctx, AV_LOG_INFO, "User Data=");
>>> +av_log(ctx, AV_LOG_INFO, "%s", user_data);
>>
>> Given that the user_data needn't be zero-terminated, you must specify
>> the precision (i.e. the number of bytes to write) here.
>>
>>> +}
>>> +}
>>> +
>>> static void dump_color_property(AVFilterContext *ctx, AVFrame *frame)
>>> {
>>> const char *color_range_str =
>>> av_color_range_name(frame->color_range);
>>> @@ -375,6 +409,9 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
>>> *frame)
>>> case AV_FRAME_DATA_VIDEO_ENC_PARAMS:
>>> dump_video_enc_params(ctx, sd);
>>> break;
>>> +case AV_FRAME_DATA_SEI_UNREGISTERED:
>>> +dump_sei_unregistered_metadata(ctx, sd);
>>> +break;
>>> default:
>>> av_log(ctx, AV_LOG_WARNING, "unknown side data type %d (%d
>>> bytes)",
>>> sd->type, sd->size);
>>>
>>
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v11 3/4] avfilter/vf_showinfo: display H.26[45] user data unregistered sei message
On Wed, Jun 10, 2020 at 08:13:59PM +0200, Andreas Rheinhardt wrote:
> lance.lmw...@gmail.com:
> > From: Limin Wang
> >
> > Signed-off-by: Limin Wang
> > ---
> > libavfilter/vf_showinfo.c | 37 +
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c
> > index 5d4aee4..2511da5 100644
> > --- a/libavfilter/vf_showinfo.c
> > +++ b/libavfilter/vf_showinfo.c
> > @@ -37,6 +37,7 @@
> > #include "libavutil/timecode.h"
> > #include "libavutil/mastering_display_metadata.h"
> > #include "libavutil/video_enc_params.h"
> > +#include "libavutil/avstring.h"
> >
> > #include "avfilter.h"
> > #include "internal.h"
> > @@ -190,6 +191,39 @@ static void dump_video_enc_params(AVFilterContext
> > *ctx, AVFrameSideData *sd)
> > av_log(ctx, AV_LOG_INFO, "%u blocks; ", par->nb_blocks);
> > }
> >
> > +static int string_is_print(const uint8_t *str)
> > +{
> > +while (av_isgraph(*str)) str++;
> > +return !*str;
>
> This is dangerous: The SEI message needn't be zero-terminated, so this
> may segfault. Furthermore, it is also wrong: If the user data payload
> happens to contain a binary zero preceded by printable ASCII characters,
> it will be considered a string, even if there are unprintable characters
> after the binary zero (which may really happen if the payload is
> actually binary).
Give below test example, if a binary zero preceded by "hello", the %s will print
out "hello" and ignore the other unprintable characters. I think it's expected.
We only dump the printable result if it is string or parial string.
unsigned char test[10];
test[0] = 'h';
test[1] = 'e';
test[2] = 'l';
test[3] = 'l';
test[4] = 'o';
test[5] = 0;
test[6] = 133;
test[7] = 144;
printf( "test: %s \n", test);
So I thbink it's safety after add one padding zero bytes for the user data
buffer.
>
> > +}
> > +
> > +static void dump_sei_unregistered_metadata(AVFilterContext *ctx,
> > AVFrameSideData *sd)
> > +{
> > +const int uuid_size = 16;
> > +uint8_t *user_data = sd->data;
> > +
> > +if (sd->size < uuid_size) {
> > +av_log(ctx, AV_LOG_ERROR, "invalid data(%d < UUID(%d-bytes))",
> > sd->size, uuid_size);
> > +return;
> > +}
> > +
> > +av_log(ctx, AV_LOG_INFO, "User Data Unregistered:\n");
> > +av_log(ctx, AV_LOG_INFO, "UUID=");
> > +for (int i = 0; i < uuid_size; i++) {
> > +av_log(ctx, AV_LOG_INFO, "%02x", user_data[i]);
> > +if (i == 3 || i == 5 || i == 7 || i == 9)
> > +av_log(ctx, AV_LOG_INFO, "-");
> > +}
> > +av_log(ctx, AV_LOG_INFO, "\n");
> > +
> > +user_data += uuid_size;
> > +/* Only print the user data details if it's string */
> > +if (string_is_print(user_data)) {
> > +av_log(ctx, AV_LOG_INFO, "User Data=");
> > +av_log(ctx, AV_LOG_INFO, "%s", user_data);
>
> Given that the user_data needn't be zero-terminated, you must specify
> the precision (i.e. the number of bytes to write) here.
>
> > +}
> > +}
> > +
> > static void dump_color_property(AVFilterContext *ctx, AVFrame *frame)
> > {
> > const char *color_range_str =
> > av_color_range_name(frame->color_range);
> > @@ -375,6 +409,9 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> > *frame)
> > case AV_FRAME_DATA_VIDEO_ENC_PARAMS:
> > dump_video_enc_params(ctx, sd);
> > break;
> > +case AV_FRAME_DATA_SEI_UNREGISTERED:
> > +dump_sei_unregistered_metadata(ctx, sd);
> > +break;
> > default:
> > av_log(ctx, AV_LOG_WARNING, "unknown side data type %d (%d
> > bytes)",
> > sd->type, sd->size);
> >
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
--
Thanks,
Limin Wang
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v11 3/4] avfilter/vf_showinfo: display H.26[45] user data unregistered sei message
On Wed, Jun 10, 2020 at 08:13:59PM +0200, Andreas Rheinhardt wrote:
> lance.lmw...@gmail.com:
> > From: Limin Wang
> >
> > Signed-off-by: Limin Wang
> > ---
> > libavfilter/vf_showinfo.c | 37 +
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c
> > index 5d4aee4..2511da5 100644
> > --- a/libavfilter/vf_showinfo.c
> > +++ b/libavfilter/vf_showinfo.c
> > @@ -37,6 +37,7 @@
> > #include "libavutil/timecode.h"
> > #include "libavutil/mastering_display_metadata.h"
> > #include "libavutil/video_enc_params.h"
> > +#include "libavutil/avstring.h"
> >
> > #include "avfilter.h"
> > #include "internal.h"
> > @@ -190,6 +191,39 @@ static void dump_video_enc_params(AVFilterContext
> > *ctx, AVFrameSideData *sd)
> > av_log(ctx, AV_LOG_INFO, "%u blocks; ", par->nb_blocks);
> > }
> >
> > +static int string_is_print(const uint8_t *str)
> > +{
> > +while (av_isgraph(*str)) str++;
> > +return !*str;
>
> This is dangerous: The SEI message needn't be zero-terminated, so this
> may segfault. Furthermore, it is also wrong: If the user data payload
> happens to contain a binary zero preceded by printable ASCII characters,
> it will be considered a string, even if there are unprintable characters
> after the binary zero (which may really happen if the payload is
> actually binary).
I'll consider to alloc one zero padding byte memory for user data to force
zero-terminated. For the second question, we'll print the printable string,
after the binary zero will ignored, I think it's expected? or do you want to
dump the left binary by hex? If so it's considerable to dump them by hex,
although it's not friendly if the data is string.
>
> > +}
> > +
> > +static void dump_sei_unregistered_metadata(AVFilterContext *ctx,
> > AVFrameSideData *sd)
> > +{
> > +const int uuid_size = 16;
> > +uint8_t *user_data = sd->data;
> > +
> > +if (sd->size < uuid_size) {
> > +av_log(ctx, AV_LOG_ERROR, "invalid data(%d < UUID(%d-bytes))",
> > sd->size, uuid_size);
> > +return;
> > +}
> > +
> > +av_log(ctx, AV_LOG_INFO, "User Data Unregistered:\n");
> > +av_log(ctx, AV_LOG_INFO, "UUID=");
> > +for (int i = 0; i < uuid_size; i++) {
> > +av_log(ctx, AV_LOG_INFO, "%02x", user_data[i]);
> > +if (i == 3 || i == 5 || i == 7 || i == 9)
> > +av_log(ctx, AV_LOG_INFO, "-");
> > +}
> > +av_log(ctx, AV_LOG_INFO, "\n");
> > +
> > +user_data += uuid_size;
> > +/* Only print the user data details if it's string */
> > +if (string_is_print(user_data)) {
> > +av_log(ctx, AV_LOG_INFO, "User Data=");
> > +av_log(ctx, AV_LOG_INFO, "%s", user_data);
>
> Given that the user_data needn't be zero-terminated, you must specify
> the precision (i.e. the number of bytes to write) here.
>
> > +}
> > +}
> > +
> > static void dump_color_property(AVFilterContext *ctx, AVFrame *frame)
> > {
> > const char *color_range_str =
> > av_color_range_name(frame->color_range);
> > @@ -375,6 +409,9 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> > *frame)
> > case AV_FRAME_DATA_VIDEO_ENC_PARAMS:
> > dump_video_enc_params(ctx, sd);
> > break;
> > +case AV_FRAME_DATA_SEI_UNREGISTERED:
> > +dump_sei_unregistered_metadata(ctx, sd);
> > +break;
> > default:
> > av_log(ctx, AV_LOG_WARNING, "unknown side data type %d (%d
> > bytes)",
> > sd->type, sd->size);
> >
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
--
Thanks,
Limin Wang
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v11 3/4] avfilter/vf_showinfo: display H.26[45] user data unregistered sei message
lance.lmw...@gmail.com:
> From: Limin Wang
>
> Signed-off-by: Limin Wang
> ---
> libavfilter/vf_showinfo.c | 37 +
> 1 file changed, 37 insertions(+)
>
> diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c
> index 5d4aee4..2511da5 100644
> --- a/libavfilter/vf_showinfo.c
> +++ b/libavfilter/vf_showinfo.c
> @@ -37,6 +37,7 @@
> #include "libavutil/timecode.h"
> #include "libavutil/mastering_display_metadata.h"
> #include "libavutil/video_enc_params.h"
> +#include "libavutil/avstring.h"
>
> #include "avfilter.h"
> #include "internal.h"
> @@ -190,6 +191,39 @@ static void dump_video_enc_params(AVFilterContext *ctx,
> AVFrameSideData *sd)
> av_log(ctx, AV_LOG_INFO, "%u blocks; ", par->nb_blocks);
> }
>
> +static int string_is_print(const uint8_t *str)
> +{
> +while (av_isgraph(*str)) str++;
> +return !*str;
This is dangerous: The SEI message needn't be zero-terminated, so this
may segfault. Furthermore, it is also wrong: If the user data payload
happens to contain a binary zero preceded by printable ASCII characters,
it will be considered a string, even if there are unprintable characters
after the binary zero (which may really happen if the payload is
actually binary).
> +}
> +
> +static void dump_sei_unregistered_metadata(AVFilterContext *ctx,
> AVFrameSideData *sd)
> +{
> +const int uuid_size = 16;
> +uint8_t *user_data = sd->data;
> +
> +if (sd->size < uuid_size) {
> +av_log(ctx, AV_LOG_ERROR, "invalid data(%d < UUID(%d-bytes))",
> sd->size, uuid_size);
> +return;
> +}
> +
> +av_log(ctx, AV_LOG_INFO, "User Data Unregistered:\n");
> +av_log(ctx, AV_LOG_INFO, "UUID=");
> +for (int i = 0; i < uuid_size; i++) {
> +av_log(ctx, AV_LOG_INFO, "%02x", user_data[i]);
> +if (i == 3 || i == 5 || i == 7 || i == 9)
> +av_log(ctx, AV_LOG_INFO, "-");
> +}
> +av_log(ctx, AV_LOG_INFO, "\n");
> +
> +user_data += uuid_size;
> +/* Only print the user data details if it's string */
> +if (string_is_print(user_data)) {
> +av_log(ctx, AV_LOG_INFO, "User Data=");
> +av_log(ctx, AV_LOG_INFO, "%s", user_data);
Given that the user_data needn't be zero-terminated, you must specify
the precision (i.e. the number of bytes to write) here.
> +}
> +}
> +
> static void dump_color_property(AVFilterContext *ctx, AVFrame *frame)
> {
> const char *color_range_str =
> av_color_range_name(frame->color_range);
> @@ -375,6 +409,9 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *frame)
> case AV_FRAME_DATA_VIDEO_ENC_PARAMS:
> dump_video_enc_params(ctx, sd);
> break;
> +case AV_FRAME_DATA_SEI_UNREGISTERED:
> +dump_sei_unregistered_metadata(ctx, sd);
> +break;
> default:
> av_log(ctx, AV_LOG_WARNING, "unknown side data type %d (%d
> bytes)",
> sd->type, sd->size);
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
