| Message ID | 20240408125950.53472-17-ffmpeg@haasn.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | Add avcodec_get_supported_config() | expand |
| 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 |
On Mon, Apr 08, 2024 at 02:57:20PM +0200, Niklas Haas wrote: > From: Niklas Haas <git@haasn.dev> > > To convert between color spaces/ranges, if needed by the codec > properties. > --- > fftools/ffmpeg_filter.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) I presume this is intended to change some cases iam asking because it does for example, this one -i aletrek.mkv -t 1 -bitexact /tmp/file-aletrek-palette.mkv has the output file change slightly https://trac.ffmpeg.org/attachment/ticket/5071/aletrek.mkv also given fate does not change, it would make sense to add a testcase to fate that does cover this thx [...]
On Wed, 10 Apr 2024 03:25:45 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Apr 08, 2024 at 02:57:20PM +0200, Niklas Haas wrote: > > From: Niklas Haas <git@haasn.dev> > > > > To convert between color spaces/ranges, if needed by the codec > > properties. > > --- > > fftools/ffmpeg_filter.c | 34 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > I presume this is intended to change some cases > iam asking because it does > for example, this one > -i aletrek.mkv -t 1 -bitexact /tmp/file-aletrek-palette.mkv > has the output file change slightly > > https://trac.ffmpeg.org/attachment/ticket/5071/aletrek.mkv > > also given fate does not change, it would make sense to add a testcase > to fate that does cover this Two notes: 1. The only difference between the `master` behavior and the new behavior is that the file is marked as limited range instead of as unspecified. However, this is the correct tagging, as the actual output *is* limited range. 2. While not *broken* per se, this is actually still a bug - in this case, since the input is basically full range, we should actually try and output full range by default. The reason it doesn't output full range here is because a consequence of the fact that format reduction happens *before* the logic in pick_format() fixes all non-YUV links to be tagged as PC/RGB-only. So the format reduction logic just sees that vf_scale can output any range in this scenario (true) and picks TV range output as the default, resulting in a conversion from the PC range input to a TV range output. The easiest solution would be to not blindly pick the first here, but to instead try and pick a colorspace and range matching the input (if one exists). But this may still break in more complicated scenarios where the dependence on the forced format spans several filters. The more correct solution would probably be to explicitly reduce only the format first (going through all the steps) and then negotiate everything that depends on the format in an entirely separate step. I'll see if I can do something about this situation, though it's ultimately not a high priority as it's not a regression compared to the status quo - just something that we could definitely improve. > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > What does censorship reveal? It reveals fear. -- Julian Assange > _______________________________________________ > 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".
On Wed, 10 Apr 2024 12:29:06 +0200 Niklas Haas <ffmpeg@haasn.xyz> wrote: > On Wed, 10 Apr 2024 03:25:45 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Mon, Apr 08, 2024 at 02:57:20PM +0200, Niklas Haas wrote: > > > From: Niklas Haas <git@haasn.dev> > > > > > > To convert between color spaces/ranges, if needed by the codec > > > properties. > > > --- > > > fftools/ffmpeg_filter.c | 34 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 34 insertions(+) > > > > I presume this is intended to change some cases > > iam asking because it does > > for example, this one > > -i aletrek.mkv -t 1 -bitexact /tmp/file-aletrek-palette.mkv > > has the output file change slightly > > > > https://trac.ffmpeg.org/attachment/ticket/5071/aletrek.mkv > > > > also given fate does not change, it would make sense to add a testcase > > to fate that does cover this > > Two notes: > > 1. The only difference between the `master` behavior and the new > behavior is that the file is marked as limited range instead of as > unspecified. However, this is the correct tagging, as the actual > output *is* limited range. > > 2. While not *broken* per se, this is actually still a bug - in this > case, since the input is basically full range, we should actually try > and output full range by default. > > The reason it doesn't output full range here is because a consequence of > the fact that format reduction happens *before* the logic in > pick_format() fixes all non-YUV links to be tagged as PC/RGB-only. So > the format reduction logic just sees that vf_scale can output any range > in this scenario (true) and picks TV range output as the default, > resulting in a conversion from the PC range input to a TV range output. > > The easiest solution would be to not blindly pick the first here, but to > instead try and pick a colorspace and range matching the input (if one > exists). But this may still break in more complicated scenarios where > the dependence on the forced format spans several filters. > > The more correct solution would probably be to explicitly reduce only > the format first (going through all the steps) and then negotiate > everything that depends on the format in an entirely separate step. > > I'll see if I can do something about this situation, though it's > ultimately not a high priority as it's not a regression compared to the > status quo - just something that we could definitely improve. Alternatively, a third simple fix would be to make buffersrc explicitly request PC range in this scenario (e.g. RGB/PAL8 input), but that feels like a sort of a hack. > > > > > thx > > > > [...] > > -- > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > What does censorship reveal? It reveals fear. -- Julian Assange > > _______________________________________________ > > 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".
On Wed, Apr 10, 2024 at 12:29:06PM +0200, Niklas Haas wrote: > On Wed, 10 Apr 2024 03:25:45 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Mon, Apr 08, 2024 at 02:57:20PM +0200, Niklas Haas wrote: > > > From: Niklas Haas <git@haasn.dev> > > > > > > To convert between color spaces/ranges, if needed by the codec > > > properties. > > > --- > > > fftools/ffmpeg_filter.c | 34 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 34 insertions(+) > > > > I presume this is intended to change some cases > > iam asking because it does > > for example, this one > > -i aletrek.mkv -t 1 -bitexact /tmp/file-aletrek-palette.mkv > > has the output file change slightly > > > > https://trac.ffmpeg.org/attachment/ticket/5071/aletrek.mkv > > > > also given fate does not change, it would make sense to add a testcase > > to fate that does cover this > > Two notes: > > 1. The only difference between the `master` behavior and the new > behavior is that the file is marked as limited range instead of as > unspecified. However, this is the correct tagging, as the actual > output *is* limited range. > > 2. While not *broken* per se, this is actually still a bug - in this > case, since the input is basically full range, we should actually try > and output full range by default. > > The reason it doesn't output full range here is because a consequence of > the fact that format reduction happens *before* the logic in > pick_format() fixes all non-YUV links to be tagged as PC/RGB-only. So > the format reduction logic just sees that vf_scale can output any range > in this scenario (true) and picks TV range output as the default, > resulting in a conversion from the PC range input to a TV range output. > > The easiest solution would be to not blindly pick the first here, but to > instead try and pick a colorspace and range matching the input (if one > exists). But this may still break in more complicated scenarios where > the dependence on the forced format spans several filters. > > The more correct solution would probably be to explicitly reduce only > the format first (going through all the steps) and then negotiate > everything that depends on the format in an entirely separate step. > > I'll see if I can do something about this situation, though it's > ultimately not a high priority as it's not a regression compared to the > status quo - just something that we could definitely improve. I have the feeling the colorspace negotiation has become a bit messy IIRC The way it was intended to work originally was to have a arbitrary filtergraph and then randomly simplify/merge bits of the filtergraph before picking "colorspaces" for each part. Then repeat this whole process a few times starting from the unsimplified graph. Then pick the best and that would be within a constant factor of the optimal solution for any arbitrary complex graph. Somehow this was never implemented as things worked okesich with just doing a single pass instead of multiple and then picking the best. I dont know how much this applies here now but i thought bringing up the original intended design was a good idea. Because it seems every problem in the negotiation is solved by adjusting the single pass steps or its orders while really it should have been multiple randomized passes. Maybe we never have to deal with complex enough filtergraphs where a single pass cant be made to work well and surely this here is not a complex one at all. But wanted to bring this up anyway because i think many people forgot or didnt even know the original idea was to do multiple passes so as to handle any arbitrary complex graph well. thx [...]
On Wed, 10 Apr 2024 14:59:15 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Apr 10, 2024 at 12:29:06PM +0200, Niklas Haas wrote: > > On Wed, 10 Apr 2024 03:25:45 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Mon, Apr 08, 2024 at 02:57:20PM +0200, Niklas Haas wrote: > > > > From: Niklas Haas <git@haasn.dev> > > > > > > > > To convert between color spaces/ranges, if needed by the codec > > > > properties. > > > > --- > > > > fftools/ffmpeg_filter.c | 34 ++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 34 insertions(+) > > > > > > I presume this is intended to change some cases > > > iam asking because it does > > > for example, this one > > > -i aletrek.mkv -t 1 -bitexact /tmp/file-aletrek-palette.mkv > > > has the output file change slightly > > > > > > https://trac.ffmpeg.org/attachment/ticket/5071/aletrek.mkv > > > > > > also given fate does not change, it would make sense to add a testcase > > > to fate that does cover this > > > > Two notes: > > > > 1. The only difference between the `master` behavior and the new > > behavior is that the file is marked as limited range instead of as > > unspecified. However, this is the correct tagging, as the actual > > output *is* limited range. > > > > 2. While not *broken* per se, this is actually still a bug - in this > > case, since the input is basically full range, we should actually try > > and output full range by default. > > > > The reason it doesn't output full range here is because a consequence of > > the fact that format reduction happens *before* the logic in > > pick_format() fixes all non-YUV links to be tagged as PC/RGB-only. So > > the format reduction logic just sees that vf_scale can output any range > > in this scenario (true) and picks TV range output as the default, > > resulting in a conversion from the PC range input to a TV range output. > > > > The easiest solution would be to not blindly pick the first here, but to > > instead try and pick a colorspace and range matching the input (if one > > exists). But this may still break in more complicated scenarios where > > the dependence on the forced format spans several filters. > > > > The more correct solution would probably be to explicitly reduce only > > the format first (going through all the steps) and then negotiate > > everything that depends on the format in an entirely separate step. > > > > I'll see if I can do something about this situation, though it's > > ultimately not a high priority as it's not a regression compared to the > > status quo - just something that we could definitely improve. > > I have the feeling the colorspace negotiation has become a bit messy > IIRC The way it was intended to work originally was to have a > arbitrary filtergraph and then randomly simplify/merge bits of the > filtergraph before picking "colorspaces" for each part. > Then repeat this whole process a few times starting from the unsimplified > graph. Then pick the best and that would be within a constant factor > of the optimal solution for any arbitrary complex graph. > > Somehow this was never implemented as things worked okesich with just > doing a single pass instead of multiple and then picking the best. > > I dont know how much this applies here now but i thought bringing > up the original intended design was a good idea. Because it seems > every problem in the negotiation is solved by adjusting the single pass > steps or its orders while really it should have been multiple randomized > passes. > Maybe we never have to deal with complex enough filtergraphs where a single > pass cant be made to work well and surely this here is not a complex one > at all. > But wanted to bring this up anyway because i think many people forgot > or didnt even know the original idea was to do multiple passes so as to > handle any arbitrary complex graph well. I think a greedy algorithm (not requiring juggling multiple random passes) can still work, just that we need to change the logic from: do // step 1 while (progress) do // step 2 while (progress) ... to: do { // step 1 // step 2 ... } while (any_progress) In any case, the realization is clear that we can no longer rely on negotiating everything at the same time, as we now have a fundamental interdependency of one negotiated component on another. (And I can easily see more such dependencies being added in the future) > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > If you think the mosad wants you dead since a long time then you are either > wrong or dead since a long time. > _______________________________________________ > 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".
Niklas Haas (12024-04-10): > I think a greedy algorithm (not requiring juggling multiple random > passes) can still work, It does not work for audio. Have you studied what works for audio? It is quite subtle and full of pitfalls. Regards,
On Wed, Apr 10, 2024 at 3:12 PM Nicolas George <george@nsup.org> wrote: > Niklas Haas (12024-04-10): > > I think a greedy algorithm (not requiring juggling multiple random > > passes) can still work, > > It does not work for audio. Have you studied what works for audio? It is > quite subtle and full of pitfalls. > This is ultra-vague. And needs further explanations. > > Regards, > > -- > Nicolas George > _______________________________________________ > 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". >
diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c index 83259416a68..a40c6f381f2 100644 --- a/fftools/ffmpeg_filter.c +++ b/fftools/ffmpeg_filter.c @@ -193,6 +193,8 @@ typedef struct OutputFilterPriv { int width, height; int sample_rate; AVChannelLayout ch_layout; + enum AVColorSpace color_space; + enum AVColorRange color_range; // time base in which the output is sent to our downstream // does not need to match the filtersink's timebase @@ -208,6 +210,8 @@ typedef struct OutputFilterPriv { const int *formats; const AVChannelLayout *ch_layouts; const int *sample_rates; + const enum AVColorSpace *color_spaces; + const enum AVColorRange *color_ranges; AVRational enc_timebase; // offset for output timestamps, in AV_TIME_BASE_Q @@ -379,6 +383,12 @@ DEF_CHOOSE_FORMAT(sample_fmts, enum AVSampleFormat, format, formats, DEF_CHOOSE_FORMAT(sample_rates, int, sample_rate, sample_rates, 0, "%d", ) +DEF_CHOOSE_FORMAT(color_spaces, enum AVColorSpace, color_space, color_spaces, + AVCOL_SPC_UNSPECIFIED, "%s", av_color_space_name); + +DEF_CHOOSE_FORMAT(color_ranges, enum AVColorRange, color_range, color_ranges, + AVCOL_RANGE_UNSPECIFIED, "%s", av_color_range_name); + static void choose_channel_layouts(OutputFilterPriv *ofp, AVBPrint *bprint) { if (av_channel_layout_check(&ofp->ch_layout)) { @@ -607,6 +617,8 @@ static OutputFilter *ofilter_alloc(FilterGraph *fg) ofilter->graph = fg; ofp->format = -1; ofp->index = fg->nb_outputs - 1; + ofp->color_space = AVCOL_SPC_UNSPECIFIED; + ofp->color_range = AVCOL_RANGE_UNSPECIFIED; return ofilter; } @@ -789,6 +801,24 @@ int ofilter_bind_ost(OutputFilter *ofilter, OutputStream *ost, ofp->formats = mjpeg_formats; } } + if (ost->enc_ctx->colorspace != AVCOL_SPC_UNSPECIFIED) { + ofp->color_space = ost->enc_ctx->colorspace; + } else { + ret = avcodec_get_supported_config(ost->enc_ctx, NULL, + AV_CODEC_CONFIG_COLOR_SPACE, 0, + (const void **) &ofp->color_spaces); + if (ret < 0) + return ret; + } + if (ost->enc_ctx->color_range != AVCOL_RANGE_UNSPECIFIED) { + ofp->color_range = ost->enc_ctx->color_range; + } else { + ret = avcodec_get_supported_config(ost->enc_ctx, NULL, + AV_CODEC_CONFIG_COLOR_RANGE, 0, + (const void **) &ofp->color_ranges); + if (ret < 0) + return ret; + } fgp->disable_conversions |= ost->keep_pix_fmt; @@ -1347,6 +1377,8 @@ static int configure_output_video_filter(FilterGraph *fg, AVFilterGraph *graph, av_assert0(!ost->keep_pix_fmt || (!ofp->format && !ofp->formats)); av_bprint_init(&bprint, 0, AV_BPRINT_SIZE_UNLIMITED); choose_pix_fmts(ofp, &bprint); + choose_color_spaces(ofp, &bprint); + choose_color_ranges(ofp, &bprint); if (!av_bprint_is_complete(&bprint)) return AVERROR(ENOMEM); if (bprint.len) { @@ -1777,6 +1809,8 @@ static int configure_filtergraph(FilterGraph *fg, FilterGraphThread *fgt) ofp->width = av_buffersink_get_w(sink); ofp->height = av_buffersink_get_h(sink); + ofp->color_space = av_buffersink_get_colorspace(sink); + ofp->color_range = av_buffersink_get_color_range(sink); // If the timing parameters are not locked yet, get the tentative values // here but don't lock them. They will only be used if no output frames
From: Niklas Haas <git@haasn.dev> To convert between color spaces/ranges, if needed by the codec properties. --- fftools/ffmpeg_filter.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)