diff mbox series

[FFmpeg-devel,4/7] avformat/avformat: add a flag to signal muxers that support storing cropping values

Message ID 20231007162503.1057-5-jamrial@gmail.com
State New
Headers show
Series Container level frame cropping parameters | expand

Commit Message

James Almer Oct. 7, 2023, 4:25 p.m. UTC 
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/avformat.h | 1 +
 libavformat/mux.c      | 8 ++++++++
 2 files changed, 9 insertions(+)

Comments

Anton Khirnov Oct. 10, 2023, 11:09 a.m. UTC | #1 
Quoting James Almer (2023-10-07 18:25:00)
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavformat/avformat.h | 1 +
>  libavformat/mux.c      | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 9e7eca007e..c099ca8a01 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -500,6 +500,7 @@ typedef struct AVProbeData {
>                                          The user or muxer can override this through
>                                          AVFormatContext.avoid_negative_ts
>                                          */
> +#define AVFMT_CROPPING     0x80000 /**< Format supports storing cropping values */

I have mixed feeelings about this patch, for a bunch of reasons:
* It is quite ad-hoc - we don't do this for other side data types, and
  this approach would not scale if we did.
* If we do want to signal this, we probably want to distinguish between
  support for global and per-packet values.
* How do you expect this to be useful to the callers? I don't see this
  flag actually being used in the ffmpeg CLI patch.
James Almer Oct. 10, 2023, 11:13 a.m. UTC | #2 
On 10/10/2023 8:09 AM, Anton Khirnov wrote:
> Quoting James Almer (2023-10-07 18:25:00)
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavformat/avformat.h | 1 +
>>   libavformat/mux.c      | 8 ++++++++
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 9e7eca007e..c099ca8a01 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -500,6 +500,7 @@ typedef struct AVProbeData {
>>                                           The user or muxer can override this through
>>                                           AVFormatContext.avoid_negative_ts
>>                                           */
>> +#define AVFMT_CROPPING     0x80000 /**< Format supports storing cropping values */
> 
> I have mixed feeelings about this patch, for a bunch of reasons:
> * It is quite ad-hoc - we don't do this for other side data types, and
>    this approach would not scale if we did.
> * If we do want to signal this, we probably want to distinguish between
>    support for global and per-packet values.

This patch came to be after some discussion from the first iteration of 
the set, where concerns about the cropping information being silently 
lost if apply_cropping was disabled during a transcoding or codec copy 
scenario where the output format didn't support storing said values.

> * How do you expect this to be useful to the callers? I don't see this
>    flag actually being used in the ffmpeg CLI patch.

It's a format flag. Muxers use it, and the generic mux.c code will print 
a warning if needed.
Anton Khirnov Oct. 10, 2023, 11:16 a.m. UTC | #3 
Quoting James Almer (2023-10-10 13:13:46)
> On 10/10/2023 8:09 AM, Anton Khirnov wrote:
> > Quoting James Almer (2023-10-07 18:25:00)
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >>   libavformat/avformat.h | 1 +
> >>   libavformat/mux.c      | 8 ++++++++
> >>   2 files changed, 9 insertions(+)
> >>
> >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> >> index 9e7eca007e..c099ca8a01 100644
> >> --- a/libavformat/avformat.h
> >> +++ b/libavformat/avformat.h
> >> @@ -500,6 +500,7 @@ typedef struct AVProbeData {
> >>                                           The user or muxer can override this through
> >>                                           AVFormatContext.avoid_negative_ts
> >>                                           */
> >> +#define AVFMT_CROPPING     0x80000 /**< Format supports storing cropping values */
> > 
> > I have mixed feeelings about this patch, for a bunch of reasons:
> > * It is quite ad-hoc - we don't do this for other side data types, and
> >    this approach would not scale if we did.
> > * If we do want to signal this, we probably want to distinguish between
> >    support for global and per-packet values.
> 
> This patch came to be after some discussion from the first iteration of 
> the set, where concerns about the cropping information being silently 
> lost if apply_cropping was disabled during a transcoding or codec copy 
> scenario where the output format didn't support storing said values.
> 
> > * How do you expect this to be useful to the callers? I don't see this
> >    flag actually being used in the ffmpeg CLI patch.
> 
> It's a format flag. Muxers use it, and the generic mux.c code will print 
> a warning if needed.

Why is it public then?
James Almer Oct. 10, 2023, 11:21 a.m. UTC | #4 
On 10/10/2023 8:16 AM, Anton Khirnov wrote:
> Quoting James Almer (2023-10-10 13:13:46)
>> On 10/10/2023 8:09 AM, Anton Khirnov wrote:
>>> Quoting James Almer (2023-10-07 18:25:00)
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>    libavformat/avformat.h | 1 +
>>>>    libavformat/mux.c      | 8 ++++++++
>>>>    2 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>>> index 9e7eca007e..c099ca8a01 100644
>>>> --- a/libavformat/avformat.h
>>>> +++ b/libavformat/avformat.h
>>>> @@ -500,6 +500,7 @@ typedef struct AVProbeData {
>>>>                                            The user or muxer can override this through
>>>>                                            AVFormatContext.avoid_negative_ts
>>>>                                            */
>>>> +#define AVFMT_CROPPING     0x80000 /**< Format supports storing cropping values */
>>>
>>> I have mixed feeelings about this patch, for a bunch of reasons:
>>> * It is quite ad-hoc - we don't do this for other side data types, and
>>>     this approach would not scale if we did.
>>> * If we do want to signal this, we probably want to distinguish between
>>>     support for global and per-packet values.
>>
>> This patch came to be after some discussion from the first iteration of
>> the set, where concerns about the cropping information being silently
>> lost if apply_cropping was disabled during a transcoding or codec copy
>> scenario where the output format didn't support storing said values.
>>
>>> * How do you expect this to be useful to the callers? I don't see this
>>>     flag actually being used in the ffmpeg CLI patch.
>>
>> It's a format flag. Muxers use it, and the generic mux.c code will print
>> a warning if needed.
> 
> Why is it public then?

So the library user can know beforehand if the cropping information will 
be lost or not, and choose accordingly. The warning is there for the 
cases where it was ignored.

I can add a check to the CLI for it, but other than to abort or outright 
ignore the user request to not apply cropping i don't see what it could 
do, as mux.c already prints a warning.
Anton Khirnov Oct. 10, 2023, 11:28 a.m. UTC | #5 
Quoting James Almer (2023-10-10 13:21:42)
> On 10/10/2023 8:16 AM, Anton Khirnov wrote:
> > Quoting James Almer (2023-10-10 13:13:46)
> >> On 10/10/2023 8:09 AM, Anton Khirnov wrote:
> >>> Quoting James Almer (2023-10-07 18:25:00)
> >>>> Signed-off-by: James Almer <jamrial@gmail.com>
> >>>> ---
> >>>>    libavformat/avformat.h | 1 +
> >>>>    libavformat/mux.c      | 8 ++++++++
> >>>>    2 files changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> >>>> index 9e7eca007e..c099ca8a01 100644
> >>>> --- a/libavformat/avformat.h
> >>>> +++ b/libavformat/avformat.h
> >>>> @@ -500,6 +500,7 @@ typedef struct AVProbeData {
> >>>>                                            The user or muxer can override this through
> >>>>                                            AVFormatContext.avoid_negative_ts
> >>>>                                            */
> >>>> +#define AVFMT_CROPPING     0x80000 /**< Format supports storing cropping values */
> >>>
> >>> I have mixed feeelings about this patch, for a bunch of reasons:
> >>> * It is quite ad-hoc - we don't do this for other side data types, and
> >>>     this approach would not scale if we did.
> >>> * If we do want to signal this, we probably want to distinguish between
> >>>     support for global and per-packet values.
> >>
> >> This patch came to be after some discussion from the first iteration of
> >> the set, where concerns about the cropping information being silently
> >> lost if apply_cropping was disabled during a transcoding or codec copy
> >> scenario where the output format didn't support storing said values.
> >>
> >>> * How do you expect this to be useful to the callers? I don't see this
> >>>     flag actually being used in the ffmpeg CLI patch.
> >>
> >> It's a format flag. Muxers use it, and the generic mux.c code will print
> >> a warning if needed.
> > 
> > Why is it public then?
> 
> So the library user can know beforehand if the cropping information will 
> be lost or not, and choose accordingly. The warning is there for the 
> cases where it was ignored.
> 
> I can add a check to the CLI for it, but other than to abort or outright 
> ignore the user request to not apply cropping i don't see what it could 
> do, as mux.c already prints a warning.

As I said above - there are MANY similar bits of information that the
muxer may or may not write. You cannot handle all of them with this
approach, and singling out cropping seems horribly ad-hoc to me.
diff mbox series

Patch

diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 9e7eca007e..c099ca8a01 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -500,6 +500,7 @@  typedef struct AVProbeData {
                                         The user or muxer can override this through
                                         AVFormatContext.avoid_negative_ts
                                         */
+#define AVFMT_CROPPING     0x80000 /**< Format supports storing cropping values */
 
 #define AVFMT_SEEK_TO_PTS   0x4000000 /**< Seeking is based on PTS */
 
diff --git a/libavformat/mux.c b/libavformat/mux.c
index c7877c5d98..c5e8a4ca74 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -188,6 +188,7 @@  static int init_muxer(AVFormatContext *s, AVDictionary **options)
     AVDictionary *tmp = NULL;
     const FFOutputFormat *of = ffofmt(s->oformat);
     AVDictionaryEntry *e;
+    int warned_crop = 0;
     int ret = 0;
 
     if (options)
@@ -276,6 +277,13 @@  FF_ENABLE_DEPRECATION_WARNINGS
                     goto fail;
                 }
             }
+            if (!warned_crop && !(s->oformat->flags & AVFMT_CROPPING) &&
+                av_packet_side_data_get(st->codecpar->coded_side_data,
+                                        st->codecpar->nb_coded_side_data,
+                                        AV_PKT_DATA_FRAME_CROPPING)) {
+                av_log(s, AV_LOG_WARNING, "Muxer does not support storing cropping values\n");
+                warned_crop = 1;
+            }
             break;
         }