Re: [FFmpeg-devel] [PATCH] fftools/ffprobe: fix printing side data components and pieces

[James Almer <jamrial@gmail.com>]

On 3/5/2024 11:34 AM, Stefano Sabatini wrote:
> On date Monday 2024-03-04 15:56:46 -0300, James Almer wrote:
>> Components and pieces are side data specific fields and there's a variable
>> amount of them.
>> They also need to be identified in some form, so print a type too.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> You can see the difference with
>>
>> ffprobe -show_frames -of compact $fate_samples/mov/dovi-p7.mp4
>> ffprobe -show_frames -of xml $fate_samples/mov/dovi-p7.mp4
>>
>>   fftools/ffprobe.c | 33 +++++++++++++++++++++++++++------
>>   1 file changed, 27 insertions(+), 6 deletions(-)
>>
>> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
>> index ac6b92f5d6..d3f27987a0 100644
>> --- a/fftools/ffprobe.c
>> +++ b/fftools/ffprobe.c
>> @@ -244,6 +244,11 @@ static const char *get_frame_side_data_type(const void *data)
>>       return av_x_if_null(av_frame_side_data_name(sd->type), "unknown");
>>   }
>>   
>> +static const char *get_raw_string_type(const void *data)
>> +{
>> +    return data;
>> +}
>> +
>>   static struct section sections[] = {
>>       [SECTION_ID_CHAPTERS] =           { SECTION_ID_CHAPTERS, "chapters", SECTION_FLAG_IS_ARRAY, { SECTION_ID_CHAPTER, -1 } },
>>       [SECTION_ID_CHAPTER] =            { SECTION_ID_CHAPTER, "chapter", 0, { SECTION_ID_CHAPTER_TAGS, -1 } },
>> @@ -258,10 +263,10 @@ static struct section sections[] = {
>>       [SECTION_ID_FRAME_SIDE_DATA] =     { SECTION_ID_FRAME_SIDE_DATA, "side_data", SECTION_FLAG_HAS_VARIABLE_FIELDS|SECTION_FLAG_HAS_TYPE, { SECTION_ID_FRAME_SIDE_DATA_TIMECODE_LIST, SECTION_ID_FRAME_SIDE_DATA_COMPONENT_LIST, -1 }, .unique_name = "frame_side_data", .element_name = "side_datum", .get_type = get_frame_side_data_type },
>>       [SECTION_ID_FRAME_SIDE_DATA_TIMECODE_LIST] =  { SECTION_ID_FRAME_SIDE_DATA_TIMECODE_LIST, "timecodes", SECTION_FLAG_IS_ARRAY, { SECTION_ID_FRAME_SIDE_DATA_TIMECODE, -1 } },
>>       [SECTION_ID_FRAME_SIDE_DATA_TIMECODE] =       { SECTION_ID_FRAME_SIDE_DATA_TIMECODE, "timecode", 0, { -1 } },
>> -    [SECTION_ID_FRAME_SIDE_DATA_COMPONENT_LIST] = { SECTION_ID_FRAME_SIDE_DATA_COMPONENT_LIST, "components", SECTION_FLAG_IS_ARRAY, { SECTION_ID_FRAME_SIDE_DATA_COMPONENT, -1 } },
>> -    [SECTION_ID_FRAME_SIDE_DATA_COMPONENT] =      { SECTION_ID_FRAME_SIDE_DATA_COMPONENT, "component", 0, { SECTION_ID_FRAME_SIDE_DATA_PIECE_LIST, -1 } },
>> -    [SECTION_ID_FRAME_SIDE_DATA_PIECE_LIST] =   { SECTION_ID_FRAME_SIDE_DATA_PIECE_LIST, "pieces", SECTION_FLAG_IS_ARRAY, { SECTION_ID_FRAME_SIDE_DATA_PIECE, -1 } },
>> -    [SECTION_ID_FRAME_SIDE_DATA_PIECE] =        { SECTION_ID_FRAME_SIDE_DATA_PIECE, "piece", 0, { -1 } },
>> +    [SECTION_ID_FRAME_SIDE_DATA_COMPONENT_LIST] = { SECTION_ID_FRAME_SIDE_DATA_COMPONENT_LIST, "components", SECTION_FLAG_IS_ARRAY, { SECTION_ID_FRAME_SIDE_DATA_COMPONENT, -1 }, .unique_name = "frame_side_data_components" },
>> +    [SECTION_ID_FRAME_SIDE_DATA_COMPONENT] =      { SECTION_ID_FRAME_SIDE_DATA_COMPONENT, "component", SECTION_FLAG_HAS_VARIABLE_FIELDS|SECTION_FLAG_HAS_TYPE, { SECTION_ID_FRAME_SIDE_DATA_PIECE_LIST, -1 }, .unique_name = "frame_side_data_component", .element_name = "component_entry", .get_type = get_raw_string_type },
>> +    [SECTION_ID_FRAME_SIDE_DATA_PIECE_LIST] =   { SECTION_ID_FRAME_SIDE_DATA_PIECE_LIST, "pieces", SECTION_FLAG_IS_ARRAY, { SECTION_ID_FRAME_SIDE_DATA_PIECE, -1 }, .unique_name = "frame_side_data_component_pieces" },
>> +    [SECTION_ID_FRAME_SIDE_DATA_PIECE] =        { SECTION_ID_FRAME_SIDE_DATA_PIECE, "piece", SECTION_FLAG_HAS_VARIABLE_FIELDS|SECTION_FLAG_HAS_TYPE, { -1 }, .unique_name = "frame_side_data_component_piece", .element_name = "piece_entry", .get_type = get_raw_string_type },
>>       [SECTION_ID_FRAME_LOGS] =         { SECTION_ID_FRAME_LOGS, "logs", SECTION_FLAG_IS_ARRAY, { SECTION_ID_FRAME_LOG, -1 } },
>>       [SECTION_ID_FRAME_LOG] =          { SECTION_ID_FRAME_LOG, "log", 0, { -1 },  },
>>       [SECTION_ID_LIBRARY_VERSIONS] =   { SECTION_ID_LIBRARY_VERSIONS, "library_versions", SECTION_FLAG_IS_ARRAY, { SECTION_ID_LIBRARY_VERSION, -1 } },
>> @@ -2063,14 +2068,30 @@ static void print_dovi_metadata(WriterContext *w, const AVDOVIMetadata *dovi)
>>   
>>           for (int c = 0; c < 3; c++) {
>>               const AVDOVIReshapingCurve *curve = &mapping->curves[c];
>> -            writer_print_section_header(w, NULL, SECTION_ID_FRAME_SIDE_DATA_COMPONENT);
>> +            writer_print_section_header(w, "Reshaping curve", SECTION_ID_FRAME_SIDE_DATA_COMPONENT);
>>   
>>               print_list_fmt("pivots", "%"PRIu16, curve->num_pivots, 1, curve->pivots[idx]);
>>   
>>               writer_print_section_header(w, NULL, SECTION_ID_FRAME_SIDE_DATA_PIECE_LIST);
>>               for (int i = 0; i < curve->num_pivots - 1; i++) {
>> +                AVBPrint piece_buf;
>> +
>> +                av_bprint_init(&piece_buf, 1, AV_BPRINT_SIZE_UNLIMITED);
>> +                switch (curve->mapping_idc[i]) {
>> +                case AV_DOVI_MAPPING_POLYNOMIAL:
>> +                    av_bprintf(&piece_buf, "Polynomial");
>> +                    break;
>> +                case AV_DOVI_MAPPING_MMR:
>> +                    av_bprintf(&piece_buf, "MMR");
>> +                    break;
>> +                default:
>> +                    av_bprintf(&piece_buf, "Unknown");
>> +                    break;
>> +                }
>> +                av_bprintf(&piece_buf, " mapping");
>>   
>> -                writer_print_section_header(w, NULL, SECTION_ID_FRAME_SIDE_DATA_PIECE);
>> +                writer_print_section_header(w, piece_buf.str, SECTION_ID_FRAME_SIDE_DATA_PIECE);
>> +                av_bprint_finalize(&piece_buf, NULL);
> 
> LGTM. Not clear if this is fixing a regression or if the format (at
> least for XML/compact) was always wrong.

I'm on the fence on this. Try removing the SECTION_FLAG_HAS_TYPE from 
SECTION_ID_FRAME_SIDE_DATA_COMPONENT and 
SECTION_ID_FRAME_SIDE_DATA_PIECE and the output is broken again for the 
compact output. So at least compact_print_section_header() may be 
faulty. I did not investigate much because I'm not familiar with the code.

In any case, this change is needed because of the 
SECTION_FLAG_HAS_VARIABLE_FIELDS addition for side data where the 
printed fields are not a fixed amount.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".