[FFmpeg-devel] ffmpeg: set user-set rotation for encoded streams too

So far, -metadata:s:v rotate would only be applied to streamcopied
video streams.
---
 fftools/ffmpeg.c | 7 +++++++
 1 file changed, 7 insertions(+)

Quoting Gyan Doshi (2022-05-10 13:40:54)
> So far, -metadata:s:v rotate would only be applied to streamcopied
> video streams.

Using -metadata for this functionality is a hack that should be removed,
not extended.

On 2022-05-11 05:26 pm, Anton Khirnov wrote:
> Quoting Gyan Doshi (2022-05-10 13:40:54)
>> So far, -metadata:s:v rotate would only be applied to streamcopied
>> video streams.
> Using -metadata for this functionality is a hack that should be removed,
> not extended.

When there's a replacement for CLI users, sure.
Till then, there's no need for the disparity to be maintained.

Regards,
Gyan

On 2022-05-10 05:10 pm, Gyan Doshi wrote:
> So far, -metadata:s:v rotate would only be applied to streamcopied
> video streams.

Plan to push tomorrow.

Gyan

> ---
>   fftools/ffmpeg.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index a85ed18b08..7c1db2162a 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -3048,6 +3048,13 @@ static int init_output_stream_encode(OutputStream *ost, AVFrame *frame)
>               av_reduce(&ost->frame_rate.num, &ost->frame_rate.den,
>                         ost->frame_rate.num, ost->frame_rate.den, 65535);
>           }
> +
> +        if (ost->rotate_overridden) {
> +            uint8_t *sd = av_stream_new_side_data(ost->st, AV_PKT_DATA_DISPLAYMATRIX,
> +                                                  sizeof(int32_t) * 9);
> +            if (sd)
> +                av_display_rotation_set((int32_t *)sd, -ost->rotate_override_value);
> +        }
>       }
>   
>       switch (enc_ctx->codec_type) {

Quoting Gyan Doshi (2022-05-11 18:30:41)
> 
> 
> On 2022-05-10 05:10 pm, Gyan Doshi wrote:
> > So far, -metadata:s:v rotate would only be applied to streamcopied
> > video streams.
> 
> Plan to push tomorrow.

WTF? This is not how development works.

I had an objection to your patch, it was not resolved yet. Threatening
to "push tomorrow" is very rude, at best.

Quoting Gyan Doshi (2022-05-11 14:18:49)
> 
> 
> On 2022-05-11 05:26 pm, Anton Khirnov wrote:
> > Quoting Gyan Doshi (2022-05-10 13:40:54)
> >> So far, -metadata:s:v rotate would only be applied to streamcopied
> >> video streams.
> > Using -metadata for this functionality is a hack that should be removed,
> > not extended.
> 
> When there's a replacement for CLI users, sure.
> Till then, there's no need for the disparity to be maintained.

I disagree. You are adding new behavior, which will need to be
maintained for backward compatibility and add extra burden on the person
who would want to implement this properly.

If you want this functionality, just add a new option. It's not that
hard. There's plenty of hacks in ffmpeg already, we don't need any new
ones.

On 2022-05-12 12:19 am, Anton Khirnov wrote:
> Quoting Gyan Doshi (2022-05-11 14:18:49)
>>
>> On 2022-05-11 05:26 pm, Anton Khirnov wrote:
>>> Quoting Gyan Doshi (2022-05-10 13:40:54)
>>>> So far, -metadata:s:v rotate would only be applied to streamcopied
>>>> video streams.
>>> Using -metadata for this functionality is a hack that should be removed,
>>> not extended.
>> When there's a replacement for CLI users, sure.
>> Till then, there's no need for the disparity to be maintained.
> I disagree. You are adding new behavior, which will need to be
> maintained for backward compatibility and add extra burden on the person
> who would want to implement this properly.
>
> If you want this functionality, just add a new option. It's not that
> hard. There's plenty of hacks in ffmpeg already, we don't need any new
> ones.

All the current state does is force the user to run a 2nd remux command 
to set rotation.
A new option for encoded streams creates a divergent syntax for no 
visible benefit.

If and when a display matrix can be user-specified, it will be easier to 
retire the metadata hack than to change the arg syntax of a newly added 
'rotate' option.

Regards,
Gyan

Quoting Gyan Doshi (2022-05-11 21:15:15)
> 
> 
> On 2022-05-12 12:19 am, Anton Khirnov wrote:
> > Quoting Gyan Doshi (2022-05-11 14:18:49)
> >>
> >> On 2022-05-11 05:26 pm, Anton Khirnov wrote:
> >>> Quoting Gyan Doshi (2022-05-10 13:40:54)
> >>>> So far, -metadata:s:v rotate would only be applied to streamcopied
> >>>> video streams.
> >>> Using -metadata for this functionality is a hack that should be removed,
> >>> not extended.
> >> When there's a replacement for CLI users, sure.
> >> Till then, there's no need for the disparity to be maintained.
> > I disagree. You are adding new behavior, which will need to be
> > maintained for backward compatibility and add extra burden on the person
> > who would want to implement this properly.
> >
> > If you want this functionality, just add a new option. It's not that
> > hard. There's plenty of hacks in ffmpeg already, we don't need any new
> > ones.
> 
> All the current state does is force the user to run a 2nd remux command 
> to set rotation.
> A new option for encoded streams creates a divergent syntax for no 
> visible benefit.

This new option would presumably apply to all streams, deprecating the
use of metadata for specifying rotations.

> If and when a display matrix can be user-specified, it will be easier to 
> retire the metadata hack than to change the arg syntax of a newly added 
> 'rotate' option.

I see no reason why the syntax should be changed.

On 2022-05-12 10:55 am, Anton Khirnov wrote:
> Quoting Gyan Doshi (2022-05-11 21:15:15)
>>
>> On 2022-05-12 12:19 am, Anton Khirnov wrote:
>>> Quoting Gyan Doshi (2022-05-11 14:18:49)
>>>> On 2022-05-11 05:26 pm, Anton Khirnov wrote:
>>>>> Quoting Gyan Doshi (2022-05-10 13:40:54)
>>>>>> So far, -metadata:s:v rotate would only be applied to streamcopied
>>>>>> video streams.
>>>>> Using -metadata for this functionality is a hack that should be removed,
>>>>> not extended.
>>>> When there's a replacement for CLI users, sure.
>>>> Till then, there's no need for the disparity to be maintained.
>>> I disagree. You are adding new behavior, which will need to be
>>> maintained for backward compatibility and add extra burden on the person
>>> who would want to implement this properly.
>>>
>>> If you want this functionality, just add a new option. It's not that
>>> hard. There's plenty of hacks in ffmpeg already, we don't need any new
>>> ones.
>> All the current state does is force the user to run a 2nd remux command
>> to set rotation.
>> A new option for encoded streams creates a divergent syntax for no
>> visible benefit.
> This new option would presumably apply to all streams, deprecating the
> use of metadata for specifying rotations.
>
>> If and when a display matrix can be user-specified, it will be easier to
>> retire the metadata hack than to change the arg syntax of a newly added
>> 'rotate' option.
> I see no reason why the syntax should be changed.

A substitute option for metadata rotate would accept a single rotation 
value.
If adding an option for matrix, we should allow flips too.

Regards,
Gyan

Quoting Gyan Doshi (2022-05-12 09:01:31)
> A substitute option for metadata rotate would accept a single rotation 
> value.
> If adding an option for matrix, we should allow flips too.

I see no reason not to have both. You can have a simple option for just
the rotation and an advanced option for specifying full matrix.

On Thu, May 12, 2022 at 11:19 AM Anton Khirnov <anton@khirnov.net> wrote:
>
> Quoting Gyan Doshi (2022-05-12 09:01:31)
> > A substitute option for metadata rotate would accept a single rotation
> > value.
> > If adding an option for matrix, we should allow flips too.
>
> I see no reason not to have both. You can have a simple option for just
> the rotation and an advanced option for specifying full matrix.
>

FYI I had thought about arguments-to-side data earlier, and as I saw
this thread I decided to make a proof-of-concept that has one layer
which constructs an AVBufferRef of the side data itself based on an
AVDictionary (constructor, basically), and then another which then
goes over type-arguments sort of dictionary and attaches things (to an
AVFrame in this case). AVPacket and AVFrame side data is slightly
different, so I guess you'd have to split one layer which generates an
AVBufferRef, and then that'd be utilized (discarded or not) by the
utilizing function.

I do wonder if AVOptions wouldn't be better for something like this,
but at least it lets you play around with the idea. My initial API
user ended up being the setparams filter.

Link to PoC branch:
https://github.com/jeeb/ffmpeg/commits/arguments_to_side_data

Usage example:
ffmpeg -v verbose -i INPUT -map 0:v:0 -vf
setparams=side_data=displaymatrix=angle=34.34,showinfo -vframes 1 -f
null -

In theory av_frame_side_data_name could be utilized for side data
naming, but in that case it would be "3x3 displaymatrix=angle=34.34",
which is why I - unfortunately - decided not to utilize it at first.

Jan

On 2022-05-15 04:29 pm, Jan Ekström wrote:
> On Thu, May 12, 2022 at 11:19 AM Anton Khirnov <anton@khirnov.net> wrote:
>> Quoting Gyan Doshi (2022-05-12 09:01:31)
>>> A substitute option for metadata rotate would accept a single rotation
>>> value.
>>> If adding an option for matrix, we should allow flips too.
>> I see no reason not to have both. You can have a simple option for just
>> the rotation and an advanced option for specifying full matrix.
>>
> FYI I had thought about arguments-to-side data earlier, and as I saw
> this thread I decided to make a proof-of-concept that has one layer
> which constructs an AVBufferRef of the side data itself based on an
> AVDictionary (constructor, basically), and then another which then
> goes over type-arguments sort of dictionary and attaches things (to an
> AVFrame in this case). AVPacket and AVFrame side data is slightly
> different, so I guess you'd have to split one layer which generates an
> AVBufferRef, and then that'd be utilized (discarded or not) by the
> utilizing function.
>
> I do wonder if AVOptions wouldn't be better for something like this,
> but at least it lets you play around with the idea. My initial API
> user ended up being the setparams filter.
>
> Link to PoC branch:
> https://github.com/jeeb/ffmpeg/commits/arguments_to_side_data
>
> Usage example:
> ffmpeg -v verbose -i INPUT -map 0:v:0 -vf
> setparams=side_data=displaymatrix=angle=34.34,showinfo -vframes 1 -f
> null -

The metadata hack uses avutil/display API to set the matrix and then 
attach it to the *stream* as side data  A filter will force encoding.
Since this is usability/display info for the consumer , it should remain 
available for streamcopy.

Regards,
Gyan

On Sun, May 15, 2022 at 2:58 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>
>
>
> On 2022-05-15 04:29 pm, Jan Ekström wrote:
> > On Thu, May 12, 2022 at 11:19 AM Anton Khirnov <anton@khirnov.net> wrote:
> >> Quoting Gyan Doshi (2022-05-12 09:01:31)
> >>> A substitute option for metadata rotate would accept a single rotation
> >>> value.
> >>> If adding an option for matrix, we should allow flips too.
> >> I see no reason not to have both. You can have a simple option for just
> >> the rotation and an advanced option for specifying full matrix.
> >>
> > FYI I had thought about arguments-to-side data earlier, and as I saw
> > this thread I decided to make a proof-of-concept that has one layer
> > which constructs an AVBufferRef of the side data itself based on an
> > AVDictionary (constructor, basically), and then another which then
> > goes over type-arguments sort of dictionary and attaches things (to an
> > AVFrame in this case). AVPacket and AVFrame side data is slightly
> > different, so I guess you'd have to split one layer which generates an
> > AVBufferRef, and then that'd be utilized (discarded or not) by the
> > utilizing function.
> >
> > I do wonder if AVOptions wouldn't be better for something like this,
> > but at least it lets you play around with the idea. My initial API
> > user ended up being the setparams filter.
> >
> > Link to PoC branch:
> > https://github.com/jeeb/ffmpeg/commits/arguments_to_side_data
> >
> > Usage example:
> > ffmpeg -v verbose -i INPUT -map 0:v:0 -vf
> > setparams=side_data=displaymatrix=angle=34.34,showinfo -vframes 1 -f
> > null -
>
> The metadata hack uses avutil/display API to set the matrix and then
> attach it to the *stream* as side data  A filter will force encoding.
> Since this is usability/display info for the consumer , it should remain
> available for streamcopy.

Yes, that is why I noted that there probably needs to be a separation
of APIs between the AVBufferRef and the "final" one. For re-encoding a
video filter would lead to the first AVFrame having it, and then that
could be utilized in the encoder initialization - just like my changes
pushing the video encoding later enabled color space information to be
passed on. For stream copy, it should be set either to the AVPacket(s)
or AVStream, yes. Anton talked about a bit stream filter but I have
not looked into that side of things.

This was just a proof-of-concept so people can see how string
arguments to side data might look, since IMHO we are badly lacking
such an interface (while the C APIs might be OK, we are clearly
lacking stuff to do string based object generation, which is more or
less required for CLI etc usage).

Jan

On Sun, May 15, 2022 at 3:21 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Sun, May 15, 2022 at 2:58 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:
> >
> >
> >
> > On 2022-05-15 04:29 pm, Jan Ekström wrote:
> > > On Thu, May 12, 2022 at 11:19 AM Anton Khirnov <anton@khirnov.net> wrote:
> > >> Quoting Gyan Doshi (2022-05-12 09:01:31)
> > >>> A substitute option for metadata rotate would accept a single rotation
> > >>> value.
> > >>> If adding an option for matrix, we should allow flips too.
> > >> I see no reason not to have both. You can have a simple option for just
> > >> the rotation and an advanced option for specifying full matrix.
> > >>
> > > FYI I had thought about arguments-to-side data earlier, and as I saw
> > > this thread I decided to make a proof-of-concept that has one layer
> > > which constructs an AVBufferRef of the side data itself based on an
> > > AVDictionary (constructor, basically), and then another which then
> > > goes over type-arguments sort of dictionary and attaches things (to an
> > > AVFrame in this case). AVPacket and AVFrame side data is slightly
> > > different, so I guess you'd have to split one layer which generates an
> > > AVBufferRef, and then that'd be utilized (discarded or not) by the
> > > utilizing function.
> > >
> > > I do wonder if AVOptions wouldn't be better for something like this,
> > > but at least it lets you play around with the idea. My initial API
> > > user ended up being the setparams filter.
> > >
> > > Link to PoC branch:
> > > https://github.com/jeeb/ffmpeg/commits/arguments_to_side_data
> > >
> > > Usage example:
> > > ffmpeg -v verbose -i INPUT -map 0:v:0 -vf
> > > setparams=side_data=displaymatrix=angle=34.34,showinfo -vframes 1 -f
> > > null -
> >
> > The metadata hack uses avutil/display API to set the matrix and then
> > attach it to the *stream* as side data  A filter will force encoding.
> > Since this is usability/display info for the consumer , it should remain
> > available for streamcopy.
>
> Yes, that is why I noted that there probably needs to be a separation
> of APIs between the AVBufferRef and the "final" one. For re-encoding a
> video filter would lead to the first AVFrame having it, and then that
> could be utilized in the encoder initialization - just like my changes
> pushing the video encoding later enabled color space information to be
> passed on. For stream copy, it should be set either to the AVPacket(s)
> or AVStream, yes. Anton talked about a bit stream filter but I have
> not looked into that side of things.
>
> This was just a proof-of-concept so people can see how string
> arguments to side data might look, since IMHO we are badly lacking
> such an interface (while the C APIs might be OK, we are clearly
> lacking stuff to do string based object generation, which is more or
> less required for CLI etc usage).

What I missed to note earlier, as we still don't have things settled
completely regarding arguments/AVOptions for constructors for side
data (not to mention the fun bits of the side data related structures
being slightly different between packets/streams and frames), it
completely makes sense to have options for specific side data to be
added to streams (or otherwise) for now.

I pushed an initial PoC of such a thing to
https://github.com/jeeb/ffmpeg/commits/ffmpeg_c_display_matrix
You can override the display matrix on both input side (which would
lead to autorotate logic etc working with it - although the side data
indeed does not get applied to the AVFrames themselves looking at the
showinfo filter's output), as well as on the output side, which only
affects the output stream specifically.

Jan