diff mbox series

[FFmpeg-devel] avfilter/vf_showinfo: use av_frame_side_data_name() to print side data names

Message ID 20230111193102.38416-1-jamrial@gmail.com
State Accepted
Commit b37795688a9bfa6d5a2e9b2535c4b10ebc14ac5d
Headers show
Series [FFmpeg-devel] avfilter/vf_showinfo: use av_frame_side_data_name() to print side data names | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

James Almer Jan. 11, 2023, 7:31 p.m. UTC 
This ensures all defined types are supported, even if only to report
their presence and print their size if a custom implementation is
not added.

Signed-off-by: James Almer <jamrial@gmail.com>
---
This patch supersedes:
[PATCH 1/2] avfilter/vf_showinfo: remove superfluous line break
[PATCH 2/2] avfilter/vf_showinfo: add support for raw Dolby Vision RPU side data

By achieving the same effect in a future proof, generic way.

 libavfilter/vf_showinfo.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

Comments

James Almer Jan. 13, 2023, 1:09 p.m. UTC | #1 
On 1/11/2023 4:31 PM, James Almer wrote:
> This ensures all defined types are supported, even if only to report
> their presence and print their size if a custom implementation is
> not added.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> This patch supersedes:
> [PATCH 1/2] avfilter/vf_showinfo: remove superfluous line break
> [PATCH 2/2] avfilter/vf_showinfo: add support for raw Dolby Vision RPU side data
> 
> By achieving the same effect in a future proof, generic way.
> 
>   libavfilter/vf_showinfo.c | 39 +++++++++++++++------------------------
>   1 file changed, 15 insertions(+), 24 deletions(-)

Will apply.
Jan Ekström Jan. 13, 2023, 2:38 p.m. UTC | #2 
On Wed, Jan 11, 2023 at 9:31 PM James Almer <jamrial@gmail.com> wrote:
>
> This ensures all defined types are supported, even if only to report
> their presence and print their size if a custom implementation is
> not added.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> This patch supersedes:
> [PATCH 1/2] avfilter/vf_showinfo: remove superfluous line break
> [PATCH 2/2] avfilter/vf_showinfo: add support for raw Dolby Vision RPU side data
>
> By achieving the same effect in a future proof, generic way.
>

I quite like this approach, and agree with the idea. It did feel weird
that we have a single location which sets human readable names for
side data types, yet it was not utilized.

I think the only drawback is that I'm pretty sure there would be some
side data types printed out now with a different name, and I do know
that some people parse the output of this filter instead of utilizing
ffprobe. Granted, we generally don't promise that log lines are a
machine readable interface. Thus this is not a blocker, but something
to keep in mind.

I wonder if at some point (at the latest when we'll start wanting to
make a generic string/dict based side data setting interface) we'll
want to have separately a "long name" (more human readable) as well as
a shorter identifier similar to AVCodecs' "name".

Jan
James Almer Jan. 13, 2023, 9:33 p.m. UTC | #3 
On 1/13/2023 11:38 AM, Jan Ekström wrote:
> On Wed, Jan 11, 2023 at 9:31 PM James Almer <jamrial@gmail.com> wrote:
>>
>> This ensures all defined types are supported, even if only to report
>> their presence and print their size if a custom implementation is
>> not added.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> This patch supersedes:
>> [PATCH 1/2] avfilter/vf_showinfo: remove superfluous line break
>> [PATCH 2/2] avfilter/vf_showinfo: add support for raw Dolby Vision RPU side data
>>
>> By achieving the same effect in a future proof, generic way.
>>
> 
> I quite like this approach, and agree with the idea. It did feel weird
> that we have a single location which sets human readable names for
> side data types, yet it was not utilized.
> 
> I think the only drawback is that I'm pretty sure there would be some
> side data types printed out now with a different name, and I do know
> that some people parse the output of this filter instead of utilizing
> ffprobe. Granted, we generally don't promise that log lines are a
> machine readable interface. Thus this is not a blocker, but something
> to keep in mind.

ffprobe is what we provide for parseable output. Also muxers like 
framehash, which are versioned and such. So yeah, av_log() output is not 
something we promise will be unchanged.

> 
> I wonder if at some point (at the latest when we'll start wanting to
> make a generic string/dict based side data setting interface) we'll
> want to have separately a "long name" (more human readable) as well as
> a shorter identifier similar to AVCodecs' "name".
> 
> Jan

Pushed.
diff mbox series

Patch

diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c
index 9b4a9fc981..d5edbd2f0a 100644
--- a/libavfilter/vf_showinfo.c
+++ b/libavfilter/vf_showinfo.c
@@ -68,7 +68,6 @@  static void dump_spherical(AVFilterContext *ctx, AVFrame *frame, const AVFrameSi
     const AVSphericalMapping *spherical = (const AVSphericalMapping *)sd->data;
     double yaw, pitch, roll;
 
-    av_log(ctx, AV_LOG_INFO, "spherical information: ");
     if (sd->size < sizeof(*spherical)) {
         av_log(ctx, AV_LOG_ERROR, "invalid data\n");
         return;
@@ -106,7 +105,6 @@  static void dump_stereo3d(AVFilterContext *ctx, const AVFrameSideData *sd)
 {
     const AVStereo3D *stereo;
 
-    av_log(ctx, AV_LOG_INFO, "stereoscopic information: ");
     if (sd->size < sizeof(*stereo)) {
         av_log(ctx, AV_LOG_ERROR, "invalid data\n");
         return;
@@ -150,7 +148,6 @@  static void dump_roi(AVFilterContext *ctx, const AVFrameSideData *sd)
     }
     nb_rois = sd->size / roi_size;
 
-    av_log(ctx, AV_LOG_INFO, "Regions Of Interest(ROI) information:\n");
     for (int i = 0; i < nb_rois; i++) {
         roi = (const AVRegionOfInterest *)(sd->data + roi_size * i);
         av_log(ctx, AV_LOG_INFO, "index: %d, region: (%d, %d) -> (%d, %d), qp offset: %d/%d.\n",
@@ -166,7 +163,6 @@  static void dump_detection_bbox(AVFilterContext *ctx, const AVFrameSideData *sd)
 
     header = (const AVDetectionBBoxHeader *)sd->data;
     nb_bboxes = header->nb_bboxes;
-    av_log(ctx, AV_LOG_INFO, "detection bounding boxes:\n");
     av_log(ctx, AV_LOG_INFO, "source: %s\n", header->source);
 
     for (int i = 0; i < nb_bboxes; i++) {
@@ -187,7 +183,6 @@  static void dump_mastering_display(AVFilterContext *ctx, const AVFrameSideData *
 {
     const AVMasteringDisplayMetadata *mastering_display;
 
-    av_log(ctx, AV_LOG_INFO, "mastering display: ");
     if (sd->size < sizeof(*mastering_display)) {
         av_log(ctx, AV_LOG_ERROR, "invalid data\n");
         return;
@@ -213,7 +208,6 @@  static void dump_dynamic_hdr_plus(AVFilterContext *ctx, AVFrameSideData *sd)
 {
     AVDynamicHDRPlus *hdr_plus;
 
-    av_log(ctx, AV_LOG_INFO, "HDR10+ metadata: ");
     if (sd->size < sizeof(*hdr_plus)) {
         av_log(ctx, AV_LOG_ERROR, "invalid data\n");
         return;
@@ -313,7 +307,6 @@  static void dump_dynamic_hdr_vivid(AVFilterContext *ctx, AVFrameSideData *sd)
 {
     AVDynamicHDRVivid *hdr_vivid;
 
-    av_log(ctx, AV_LOG_INFO, "HDR Vivid metadata: ");
     if (sd->size < sizeof(*hdr_vivid)) {
         av_log(ctx, AV_LOG_ERROR, "invalid hdr vivid data\n");
         return;
@@ -396,7 +389,7 @@  static void dump_content_light_metadata(AVFilterContext *ctx, AVFrameSideData *s
 {
     const AVContentLightMetadata *metadata = (const AVContentLightMetadata *)sd->data;
 
-    av_log(ctx, AV_LOG_INFO, "Content Light Level information: "
+    av_log(ctx, AV_LOG_INFO,
            "MaxCLL=%d, MaxFALL=%d",
            metadata->MaxCLL, metadata->MaxFALL);
 }
@@ -406,7 +399,7 @@  static void dump_video_enc_params(AVFilterContext *ctx, const AVFrameSideData *s
     const AVVideoEncParams *par = (const AVVideoEncParams *)sd->data;
     int plane, acdc;
 
-    av_log(ctx, AV_LOG_INFO, "video encoding parameters: type %d; ", par->type);
+    av_log(ctx, AV_LOG_INFO, "type %d; ", par->type);
     if (par->qp)
         av_log(ctx, AV_LOG_INFO, "qp=%d; ", par->qp);
     for (plane = 0; plane < FF_ARRAY_ELEMS(par->delta_qp); plane++)
@@ -430,7 +423,6 @@  static void dump_sei_unregistered_metadata(AVFilterContext *ctx, const AVFrameSi
         return;
     }
 
-    av_log(ctx, AV_LOG_INFO, "User Data Unregistered:\n");
     av_log(ctx, AV_LOG_INFO, "UUID=" AV_PRI_UUID "\n", AV_UUID_ARG(user_data));
 
     av_log(ctx, AV_LOG_INFO, "User Data=");
@@ -453,7 +445,7 @@  static void dump_sei_film_grain_params_metadata(AVFilterContext *ctx, const AVFr
         return;
     }
 
-    av_log(ctx, AV_LOG_INFO, "film grain parameters: type %s; ", film_grain_type_names[fgp->type]);
+    av_log(ctx, AV_LOG_INFO, "type %s; ", film_grain_type_names[fgp->type]);
     av_log(ctx, AV_LOG_INFO, "seed=%"PRIu64"; ", fgp->seed);
 
     switch (fgp->type) {
@@ -513,7 +505,6 @@  static void dump_dovi_metadata(AVFilterContext *ctx, const AVFrameSideData *sd)
     const AVDOVIDataMapping *mapping = av_dovi_get_mapping(dovi);
     const AVDOVIColorMetadata *color = av_dovi_get_color(dovi);
 
-    av_log(ctx, AV_LOG_INFO, "Dolby Vision Metadata:\n");
     av_log(ctx, AV_LOG_INFO, "    rpu_type=%"PRIu8"; ", hdr->rpu_type);
     av_log(ctx, AV_LOG_INFO, "rpu_format=%"PRIu16"; ", hdr->rpu_format);
     av_log(ctx, AV_LOG_INFO, "vdr_rpu_profile=%"PRIu8"; ", hdr->vdr_rpu_profile);
@@ -747,16 +738,12 @@  static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
 
     for (i = 0; i < frame->nb_side_data; i++) {
         AVFrameSideData *sd = frame->side_data[i];
+        const char *name = av_frame_side_data_name(sd->type);
 
         av_log(ctx, AV_LOG_INFO, "  side data - ");
+        if (name)
+            av_log(ctx, AV_LOG_INFO, "%s: ", name);
         switch (sd->type) {
-        case AV_FRAME_DATA_PANSCAN:
-            av_log(ctx, AV_LOG_INFO, "pan/scan");
-            break;
-        case AV_FRAME_DATA_A53_CC:
-            av_log(ctx, AV_LOG_INFO, "A/53 closed captions "
-                   "(%"SIZE_SPECIFIER" bytes)", sd->size);
-            break;
         case AV_FRAME_DATA_SPHERICAL:
             dump_spherical(ctx, frame, sd);
             break;
@@ -768,11 +755,11 @@  static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
             break;
         }
         case AV_FRAME_DATA_DISPLAYMATRIX:
-            av_log(ctx, AV_LOG_INFO, "displaymatrix: rotation of %.2f degrees",
+            av_log(ctx, AV_LOG_INFO, "rotation of %.2f degrees",
                    av_display_rotation_get((int32_t *)sd->data));
             break;
         case AV_FRAME_DATA_AFD:
-            av_log(ctx, AV_LOG_INFO, "afd: value of %"PRIu8, sd->data[0]);
+            av_log(ctx, AV_LOG_INFO, "value of %"PRIu8, sd->data[0]);
             break;
         case AV_FRAME_DATA_REGIONS_OF_INTEREST:
             dump_roi(ctx, sd);
@@ -795,7 +782,7 @@  static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
         case AV_FRAME_DATA_GOP_TIMECODE: {
             char tcbuf[AV_TIMECODE_STR_SIZE];
             av_timecode_make_mpeg_tc_string(tcbuf, *(int64_t *)(sd->data));
-            av_log(ctx, AV_LOG_INFO, "GOP timecode - %s", tcbuf);
+            av_log(ctx, AV_LOG_INFO, "%s", tcbuf);
             break;
         }
         case AV_FRAME_DATA_VIDEO_ENC_PARAMS:
@@ -811,8 +798,12 @@  static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
             dump_dovi_metadata(ctx, sd);
             break;
         default:
-            av_log(ctx, AV_LOG_WARNING, "unknown side data type %d "
-                   "(%"SIZE_SPECIFIER" bytes)\n", sd->type, sd->size);
+            if (name)
+                av_log(ctx, AV_LOG_INFO,
+                       "(%"SIZE_SPECIFIER" bytes)", sd->size);
+            else
+                av_log(ctx, AV_LOG_WARNING, "unknown side data type %d "
+                       "(%"SIZE_SPECIFIER" bytes)", sd->type, sd->size);
             break;
         }