mbox series

[FFmpeg-devel,v4,0/5] ffmpeg: late A/V encoder init, AVFrame metadata usage

Message ID 20201027183056.17993-1-jeebjp@gmail.com
Headers show
Series ffmpeg: late A/V encoder init, AVFrame metadata usage
Related show

Message

Jan Ekström Oct. 27, 2020, 6:30 p.m. UTC
This patch set started with a very simple wish to not have to set color
related values manually each time when utilizing ffmpeg.c.

As of the fourth iteration, the following changes were done since the third:
1. The data size threshold patch was moved to be the first one, thus meaning
   that there is no case of it not being applied, and the encoder initialization
   being moved later.
2. As noted by Anton, as the encoder options are applied after the
   AVFrame-based configuration, that code can be simplified to just
   simple passing of values instead of first checking if the option is
   set in the dictionary.
3. Interlaced/progressive and field order decision making commit has been
   reworded to include an explanation of the FATE test changes.

Unfortunately, audio still needs two locations where the encoder is
initialized, due to how avfilter_graph_request_oldest peeks and already puts
one AVFrame to be available from the filter graph (which is then utilized
as-is as an early return inside both av_buffersink_get_frame_flags and
av_buffersink_get_samples). If this would be improved in lavfi (or the call
to avfilter_graph_request_oldest removed), we could at least remove one of
these.

Currently limited to using values for video and started with the basic values,
more can be added later if needed.

This probably fixes some trac issues, but with a quick look I couldn't find
anything that explicitly was due to lack of video color metadata passthrough.


Jan


Example 1:

I have an RGB 3-D render, which I would like to encode into BT.709 YCbCr.
The video filter I'm generally using for this (zscale) does flag the matrix in
the output AVFrame.

Yet to have the video encoder have the correct metadata set, I have to
set the value(s) manually.

With this patch set, the value(s) from the first AVFrame fed to do_video_out
will be utilized.

Example 2:

I have an input video that sets one or more of the following:
matrix/primaries/transfer function/range/chroma location.

I just want to re-encode it. All of this metadata gets stripped.

With this patch set, the value(s) from the first AVFrame fed to do_video_out
will be utilized.

Example 3:

I have a video which has incorrect metadata tagged. Before, I had to set
the correct data data manually.

With this patch set, since ffmpeg.c takes color related options as dictionary
keys, the AVFrame values will only be utilized if the user has not set the
option for a given stream. Thus, this use case still works.

Jan Ekström (5):
  ffmpeg: add a data size threshold for muxing queue size
  ffmpeg: move AVFrame time base adjustment into a function
  ffmpeg: move A/V non-streamcopy initialization to a later point
  ffmpeg: pass decoded or filtered AVFrame to output stream
    initialization
  ffmpeg: move field order decision making to encoder initialization

 doc/ffmpeg.texi                               |   5 +
 fftools/ffmpeg.c                              | 237 ++++++++++++------
 fftools/ffmpeg.h                              |  11 +
 fftools/ffmpeg_opt.c                          |   8 +
 .../fate/concat-demuxer-extended-lavf-mxf_d10 |   2 +-
 .../fate/concat-demuxer-simple1-lavf-mxf_d10  |   2 +-
 tests/ref/fate/rgb24-mkv                      |   4 +-
 tests/ref/lavf/mxf_d10                        |   2 +-
 tests/ref/lavf/mxf_dv25                       |   2 +-
 tests/ref/lavf/mxf_dvcpro50                   |   2 +-
 tests/ref/lavf/mxf_opatom                     |   2 +-
 11 files changed, 192 insertions(+), 85 deletions(-)

Comments

Jan Ekström Oct. 28, 2020, 2:58 p.m. UTC | #1
On Tue, Oct 27, 2020 at 8:30 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> This patch set started with a very simple wish to not have to set color
> related values manually each time when utilizing ffmpeg.c.
>
> As of the fourth iteration, the following changes were done since the third:
> 1. The data size threshold patch was moved to be the first one, thus meaning
>    that there is no case of it not being applied, and the encoder initialization
>    being moved later.
> 2. As noted by Anton, as the encoder options are applied after the
>    AVFrame-based configuration, that code can be simplified to just
>    simple passing of values instead of first checking if the option is
>    set in the dictionary.
> 3. Interlaced/progressive and field order decision making commit has been
>    reworded to include an explanation of the FATE test changes.
>
> Unfortunately, audio still needs two locations where the encoder is
> initialized, due to how avfilter_graph_request_oldest peeks and already puts
> one AVFrame to be available from the filter graph (which is then utilized
> as-is as an early return inside both av_buffersink_get_frame_flags and
> av_buffersink_get_samples). If this would be improved in lavfi (or the call
> to avfilter_graph_request_oldest removed), we could at least remove one of
> these.
>
> Currently limited to using values for video and started with the basic values,
> more can be added later if needed.
>
> This probably fixes some trac issues, but with a quick look I couldn't find
> anything that explicitly was due to lack of video color metadata passthrough.
>

Received an approval from Anton, so unless there are objections I will
be pulling this set in tomorrow evening.

For those still wanting to test out and preferring to utilize git for
getting the patch set,
https://github.com/jeeb/ffmpeg/commits/late_encoder_init_v7 is
available.

Jan
Jan Ekström Oct. 29, 2020, 3:35 p.m. UTC | #2
On Wed, Oct 28, 2020 at 4:58 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Tue, Oct 27, 2020 at 8:30 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >
> > This patch set started with a very simple wish to not have to set color
> > related values manually each time when utilizing ffmpeg.c.
> >
> > As of the fourth iteration, the following changes were done since the third:
> > 1. The data size threshold patch was moved to be the first one, thus meaning
> >    that there is no case of it not being applied, and the encoder initialization
> >    being moved later.
> > 2. As noted by Anton, as the encoder options are applied after the
> >    AVFrame-based configuration, that code can be simplified to just
> >    simple passing of values instead of first checking if the option is
> >    set in the dictionary.
> > 3. Interlaced/progressive and field order decision making commit has been
> >    reworded to include an explanation of the FATE test changes.
> >
> > Unfortunately, audio still needs two locations where the encoder is
> > initialized, due to how avfilter_graph_request_oldest peeks and already puts
> > one AVFrame to be available from the filter graph (which is then utilized
> > as-is as an early return inside both av_buffersink_get_frame_flags and
> > av_buffersink_get_samples). If this would be improved in lavfi (or the call
> > to avfilter_graph_request_oldest removed), we could at least remove one of
> > these.
> >
> > Currently limited to using values for video and started with the basic values,
> > more can be added later if needed.
> >
> > This probably fixes some trac issues, but with a quick look I couldn't find
> > anything that explicitly was due to lack of video color metadata passthrough.
> >
>
> Received an approval from Anton, so unless there are objections I will
> be pulling this set in tomorrow evening.
>

Applied patch set as:
453b2f3c154f6b83221940ad411599ded91f7413
3360c9a5679cc9bd83ab5860757ebab8f64ea8ba
67be1ce0c6de330b1c10d1d121819d8a74a7b1f5
7369595c556516b90bf3acdf85043dc11add7d89
fbb44bc51a647862eb05ae3f9d7d49a0be9bed57

Thanks for testing and reviews to everyone who participated,
especially Michael and Anton.

Jan