| Message ID | 20180221222538.7255-2-philipl@overt.org |
|---|---|
| State | New |
| Headers | show |
Philip Langdale (2018-02-21): > As part of achieving our YUVJ-free future, it needs to be possible > to pass the colour range property from a decoder context to an > encoder context. In the absence of filters, this is obviously > trivial, but as soon as filters are introduced, there needs to > be a way to pass and preserve the property (or modify it, as > some filters will do). > > Based on existing properties of this type, this change adds a > link property and ways to set it from a buffer source and get it > from a buffer sink. I am not sure when I will be able to give a close look at the code, so I make my general comment immediately: Are you sure this is a property that must be forwarded and not negotiated? You seem to say that encoders may support a limited set of ranges. It looks like the range should be constrained by the output, like the pixel format, and negotiated on the chain to insert a conversion filter if necessary. But I am not familiar with color ranges. I just want to make sure you have considered the question. Regards,
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Wed, 21 Feb 2018 23:37:57 +0100 Nicolas George <george@nsup.org> wrote: > Philip Langdale (2018-02-21): > > As part of achieving our YUVJ-free future, it needs to be possible > > to pass the colour range property from a decoder context to an > > encoder context. In the absence of filters, this is obviously > > trivial, but as soon as filters are introduced, there needs to > > be a way to pass and preserve the property (or modify it, as > > some filters will do). > > > > Based on existing properties of this type, this change adds a > > link property and ways to set it from a buffer source and get it > > from a buffer sink. > > I am not sure when I will be able to give a close look at the code, > so I make my general comment immediately: > > Are you sure this is a property that must be forwarded and not > negotiated? > > You seem to say that encoders may support a limited set of ranges. It > looks like the range should be constrained by the output, like the > pixel format, and negotiated on the chain to insert a conversion > filter if necessary. > > But I am not familiar with color ranges. I just want to make sure you > have considered the question. > > Regards, > Negotiation is part of Paul's larger changeset, and will be a useful feature. My change is still a strict improvement over the current state of the world - where range is not propagated at all, regardless of compatibility. In those situations where negotiation is required, the status quo will essentially continue, with the range value not accurately reflecting the frame contents. - --phil -----BEGIN PGP SIGNATURE----- iEYEARECAAYFAlqN9woACgkQ+NaxlGp1aC6DvgCfaDsw2OsmY0xkXgajn6t7vWKx /XsAoIl2B760kvv8PPRxWdkGUbV/tCdv =F60C -----END PGP SIGNATURE-----
Philip Langdale (2018-02-21): > Negotiation is part of Paul's larger changeset, and will be a useful > feature. My change is still a strict improvement over the current state > of the world - where range is not propagated at all, regardless of > compatibility. In those situations where negotiation is required, the > status quo will essentially continue, with the range value not > accurately reflecting the frame contents. I am not comfortable with what you write here. I am afraid that adding negotiation on top of this would be more work than adding negotiation on top of the current code. I am also afraid that an incorrect value is worse than an unspecified one. But it all depends on what filters and codecs actually do with the color range, and that I do not know. Could you perhaps make a little summary of that issue: where the color range comes from, which filters and encoders do not care, which ones only work with one, which ones do something special with it? Maybe as a longer doxy comment for enum AVColorRange in libavutil/pixfmt.h? (Also, my mail contained "reply-to: ffmpeg-devel"; please heed it in the future.) Regards,
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thu, 22 Feb 2018 12:39:16 +0100 Nicolas George <george@nsup.org> wrote: > Philip Langdale (2018-02-21): > > Negotiation is part of Paul's larger changeset, and will be a useful > > feature. My change is still a strict improvement over the current > > state of the world - where range is not propagated at all, > > regardless of compatibility. In those situations where negotiation > > is required, the status quo will essentially continue, with the > > range value not accurately reflecting the frame contents. > > I am not comfortable with what you write here. > > I am afraid that adding negotiation on top of this would be more work > than adding negotiation on top of the current code. > > I am also afraid that an incorrect value is worse than an unspecified > one. > > But it all depends on what filters and codecs actually do with the > color range, and that I do not know. > > Could you perhaps make a little summary of that issue: where the color > range comes from, which filters and encoders do not care, which ones > only work with one, which ones do something special with it? Maybe as > a longer doxy comment for enum AVColorRange in libavutil/pixfmt.h? You can go back and read through Paul's patchset from december, which implements negotiation. My changes here are a strict subset of those, so empirically, merging this subset on its own does not make negotiation harder. You are also welcome to recommend merging his full patchset now; it never got the reviews it needed, but that doesn't mean it can't. Thanks, - --phil -----BEGIN PGP SIGNATURE----- iEYEARECAAYFAlqO7JYACgkQ+NaxlGp1aC7HKQCgzVxU3TSTxQeZgNA5YdZOEzaE Go0An1Jla+EPtaWOENJRcETeJh2SG3l6 =lX4D -----END PGP SIGNATURE-----
Hi, On Thu, Feb 22, 2018 at 4:15 PM, Philip Langdale <philipl@overt.org> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On Thu, 22 Feb 2018 12:39:16 +0100 > Nicolas George <george@nsup.org> wrote: > >> Philip Langdale (2018-02-21): >> > Negotiation is part of Paul's larger changeset, and will be a useful >> > feature. My change is still a strict improvement over the current >> > state of the world - where range is not propagated at all, >> > regardless of compatibility. In those situations where negotiation >> > is required, the status quo will essentially continue, with the >> > range value not accurately reflecting the frame contents. >> >> I am not comfortable with what you write here. >> >> I am afraid that adding negotiation on top of this would be more work >> than adding negotiation on top of the current code. >> >> I am also afraid that an incorrect value is worse than an unspecified >> one. >> >> But it all depends on what filters and codecs actually do with the >> color range, and that I do not know. >> >> Could you perhaps make a little summary of that issue: where the color >> range comes from, which filters and encoders do not care, which ones >> only work with one, which ones do something special with it? Maybe as >> a longer doxy comment for enum AVColorRange in libavutil/pixfmt.h? > > You can go back and read through Paul's patchset from december, which > implements negotiation. My changes here are a strict subset of those, > so empirically, merging this subset on its own does not make > negotiation harder. You are also welcome to recommend merging his full > patchset now; it never got the reviews it needed, but that doesn't mean > it can't. > I am hoping to test out this patchset in the hopes that it could rectify the issue we're having with colour metadata in a MOV/ProRes file not being migrated over to Matroska: https://ffmpeg.org/pipermail/ffmpeg-user/2018-July/040717.html I have two questions as I'm a bit confused: 1. Does this patchset require Paul's patchset to be merged? I can't tell if it has already or not. 2. I see that this thread describes a series of 4 patches, but there also seems to be a set of three patches from February - do the set of 3 supercede this set of 4? https://patchwork.ffmpeg.org/patch/7694/ Best, Kieran O'Leary IFI Irish Film Archive
On 7/25/18, Kieran O Leary <kieran.o.leary@gmail.com> wrote: > Hi, > > On Thu, Feb 22, 2018 at 4:15 PM, Philip Langdale <philipl@overt.org> wrote: >> -----BEGIN PGP SIGNED MESSAGE----- >> Hash: SHA1 >> >> On Thu, 22 Feb 2018 12:39:16 +0100 >> Nicolas George <george@nsup.org> wrote: >> >>> Philip Langdale (2018-02-21): >>> > Negotiation is part of Paul's larger changeset, and will be a useful >>> > feature. My change is still a strict improvement over the current >>> > state of the world - where range is not propagated at all, >>> > regardless of compatibility. In those situations where negotiation >>> > is required, the status quo will essentially continue, with the >>> > range value not accurately reflecting the frame contents. >>> >>> I am not comfortable with what you write here. >>> >>> I am afraid that adding negotiation on top of this would be more work >>> than adding negotiation on top of the current code. >>> >>> I am also afraid that an incorrect value is worse than an unspecified >>> one. >>> >>> But it all depends on what filters and codecs actually do with the >>> color range, and that I do not know. >>> >>> Could you perhaps make a little summary of that issue: where the color >>> range comes from, which filters and encoders do not care, which ones >>> only work with one, which ones do something special with it? Maybe as >>> a longer doxy comment for enum AVColorRange in libavutil/pixfmt.h? >> >> You can go back and read through Paul's patchset from december, which >> implements negotiation. My changes here are a strict subset of those, >> so empirically, merging this subset on its own does not make >> negotiation harder. You are also welcome to recommend merging his full >> patchset now; it never got the reviews it needed, but that doesn't mean >> it can't. >> > > I am hoping to test out this patchset in the hopes that it could > rectify the issue we're having with colour metadata in a MOV/ProRes > file not being migrated over to Matroska: > https://ffmpeg.org/pipermail/ffmpeg-user/2018-July/040717.html > > I have two questions as I'm a bit confused: > 1. Does this patchset require Paul's patchset to be merged? I can't > tell if it has already or not. > 2. I see that this thread describes a series of 4 patches, but there > also seems to be > a set of three patches from February - do the set of 3 supercede this > set of 4? https://patchwork.ffmpeg.org/patch/7694/ > IIRC this patchset have been applied, you have color range in lavfi filtergraph. But it is not used for negotiation.... Also this patchset only plays with color range and other properties are not touched.
diff --git a/doc/APIchanges b/doc/APIchanges index a98475366d..1c583420d1 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,11 @@ libavutil: 2017-10-21 API changes, most recent first: +2018-02-xx - xxxxxxx - lavfi 7.13.100 - avfilter.h + + Add AVFilterLink.color_range, and mechanisms to set/get it on + buffersrc and buffersink. + 2018-02-xx - xxxxxxx Change av_ripemd_update(), av_murmur3_update() and av_hash_update() length parameter type to size_t at next major bump. diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c index 7553f7c36a..9b93e8177e 100644 --- a/libavfilter/avfilter.c +++ b/libavfilter/avfilter.c @@ -338,6 +338,8 @@ int avfilter_config_links(AVFilterContext *filter) link->w = inlink->w; if (!link->h) link->h = inlink->h; + if (link->color_range == AVCOL_RANGE_UNSPECIFIED) + link->color_range = inlink->color_range; } else if (!link->w || !link->h) { av_log(link->src, AV_LOG_ERROR, "Video source filters must set their output link's " diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h index 2d1195eeeb..ae3287a69e 100644 --- a/libavfilter/avfilter.h +++ b/libavfilter/avfilter.h @@ -600,6 +600,8 @@ struct AVFilterLink { */ AVBufferRef *hw_frames_ctx; + enum AVColorRange color_range; ///< agreed upon video color range + #ifndef FF_INTERNAL_FIELDS /** diff --git a/libavfilter/buffersink.c b/libavfilter/buffersink.c index 0f87b5439a..897396cac4 100644 --- a/libavfilter/buffersink.c +++ b/libavfilter/buffersink.c @@ -194,6 +194,7 @@ MAKE_AVFILTERLINK_ACCESSOR(AVRational , frame_rate ) MAKE_AVFILTERLINK_ACCESSOR(int , w ) MAKE_AVFILTERLINK_ACCESSOR(int , h ) MAKE_AVFILTERLINK_ACCESSOR(AVRational , sample_aspect_ratio) +MAKE_AVFILTERLINK_ACCESSOR(enum AVColorRange, color_range ) MAKE_AVFILTERLINK_ACCESSOR(int , channels ) MAKE_AVFILTERLINK_ACCESSOR(uint64_t , channel_layout ) diff --git a/libavfilter/buffersink.h b/libavfilter/buffersink.h index 21d6bb505b..d91e3fe448 100644 --- a/libavfilter/buffersink.h +++ b/libavfilter/buffersink.h @@ -114,6 +114,7 @@ AVRational av_buffersink_get_frame_rate (const AVFilterContext *c int av_buffersink_get_w (const AVFilterContext *ctx); int av_buffersink_get_h (const AVFilterContext *ctx); AVRational av_buffersink_get_sample_aspect_ratio (const AVFilterContext *ctx); +enum AVColorRange av_buffersink_get_color_range (const AVFilterContext *ctx); int av_buffersink_get_channels (const AVFilterContext *ctx); uint64_t av_buffersink_get_channel_layout (const AVFilterContext *ctx); diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c index cd56f8ca45..2517f10feb 100644 --- a/libavfilter/buffersrc.c +++ b/libavfilter/buffersrc.c @@ -52,6 +52,7 @@ typedef struct BufferSourceContext { int w, h; enum AVPixelFormat pix_fmt; AVRational pixel_aspect; + int color_range; char *sws_param; AVBufferRef *hw_frames_ctx; @@ -111,6 +112,8 @@ int av_buffersrc_parameters_set(AVFilterContext *ctx, AVBufferSrcParameters *par s->pixel_aspect = param->sample_aspect_ratio; if (param->frame_rate.num > 0 && param->frame_rate.den > 0) s->frame_rate = param->frame_rate; + if (param->color_range > AVCOL_RANGE_UNSPECIFIED) + s->color_range = param->color_range; if (param->hw_frames_ctx) { av_buffer_unref(&s->hw_frames_ctx); s->hw_frames_ctx = av_buffer_ref(param->hw_frames_ctx); @@ -279,13 +282,19 @@ static av_cold int init_video(AVFilterContext *ctx) return AVERROR(EINVAL); } + if (!av_color_range_name(c->color_range)) { + av_log(ctx, AV_LOG_ERROR, "Invalid color_range parameter provided.\n"); + return AVERROR(EINVAL); + } + if (!(c->fifo = av_fifo_alloc(sizeof(AVFrame*)))) return AVERROR(ENOMEM); - av_log(ctx, AV_LOG_VERBOSE, "w:%d h:%d pixfmt:%s tb:%d/%d fr:%d/%d sar:%d/%d sws_param:%s\n", + av_log(ctx, AV_LOG_VERBOSE, "w:%d h:%d pixfmt:%s tb:%d/%d fr:%d/%d sar:%d/%d sws_param:%s\n color_range:%s\n", c->w, c->h, av_get_pix_fmt_name(c->pix_fmt), c->time_base.num, c->time_base.den, c->frame_rate.num, c->frame_rate.den, - c->pixel_aspect.num, c->pixel_aspect.den, (char *)av_x_if_null(c->sws_param, "")); + c->pixel_aspect.num, c->pixel_aspect.den, (char *)av_x_if_null(c->sws_param, ""), + av_color_range_name(c->color_range)); c->warning_limit = 100; return 0; } @@ -309,6 +318,15 @@ static const AVOption buffer_options[] = { { "time_base", NULL, OFFSET(time_base), AV_OPT_TYPE_RATIONAL, { .dbl = 0 }, 0, DBL_MAX, V }, { "frame_rate", NULL, OFFSET(frame_rate), AV_OPT_TYPE_RATIONAL, { .dbl = 0 }, 0, DBL_MAX, V }, { "sws_param", NULL, OFFSET(sws_param), AV_OPT_TYPE_STRING, .flags = V }, + { "color_range", NULL, OFFSET(color_range), AV_OPT_TYPE_INT, { .i64 = AVCOL_RANGE_UNSPECIFIED }, 0, INT_MAX, V, "range" }, + { "unspecified", NULL, 0, AV_OPT_TYPE_CONST, {.i64=AVCOL_RANGE_UNSPECIFIED}, 0, 0, V, "range"}, + { "unknown", NULL, 0, AV_OPT_TYPE_CONST, {.i64=AVCOL_RANGE_UNSPECIFIED}, 0, 0, V, "range"}, + { "limited", NULL, 0, AV_OPT_TYPE_CONST, {.i64=AVCOL_RANGE_MPEG}, 0, 0, V, "range"}, + { "tv", NULL, 0, AV_OPT_TYPE_CONST, {.i64=AVCOL_RANGE_MPEG}, 0, 0, V, "range"}, + { "mpeg", NULL, 0, AV_OPT_TYPE_CONST, {.i64=AVCOL_RANGE_MPEG}, 0, 0, V, "range"}, + { "full", NULL, 0, AV_OPT_TYPE_CONST, {.i64=AVCOL_RANGE_JPEG}, 0, 0, V, "range"}, + { "pc", NULL, 0, AV_OPT_TYPE_CONST, {.i64=AVCOL_RANGE_JPEG}, 0, 0, V, "range"}, + { "jpeg", NULL, 0, AV_OPT_TYPE_CONST, {.i64=AVCOL_RANGE_JPEG}, 0, 0, V, "range"}, { NULL }, }; @@ -434,6 +452,7 @@ static int config_props(AVFilterLink *link) link->w = c->w; link->h = c->h; link->sample_aspect_ratio = c->pixel_aspect; + link->color_range = c->color_range; if (c->hw_frames_ctx) { link->hw_frames_ctx = av_buffer_ref(c->hw_frames_ctx); diff --git a/libavfilter/buffersrc.h b/libavfilter/buffersrc.h index 0652113f2b..0ea2e436b3 100644 --- a/libavfilter/buffersrc.h +++ b/libavfilter/buffersrc.h @@ -114,6 +114,11 @@ typedef struct AVBufferSrcParameters { * Audio only, the audio channel layout */ uint64_t channel_layout; + + /** + * Video only, The color range of the video. + */ + enum AVColorRange color_range; } AVBufferSrcParameters; /** diff --git a/libavfilter/version.h b/libavfilter/version.h index ca096962bb..babb4187b4 100644 --- a/libavfilter/version.h +++ b/libavfilter/version.h @@ -30,7 +30,7 @@ #include "libavutil/version.h" #define LIBAVFILTER_VERSION_MAJOR 7 -#define LIBAVFILTER_VERSION_MINOR 12 +#define LIBAVFILTER_VERSION_MINOR 13 #define LIBAVFILTER_VERSION_MICRO 100 #define LIBAVFILTER_VERSION_INT AV_VERSION_INT(LIBAVFILTER_VERSION_MAJOR, \ diff --git a/libavfilter/video.c b/libavfilter/video.c index 7a8e587798..2cfe9c8f17 100644 --- a/libavfilter/video.c +++ b/libavfilter/video.c @@ -92,6 +92,7 @@ AVFrame *ff_default_get_video_buffer(AVFilterLink *link, int w, int h) return NULL; frame->sample_aspect_ratio = link->sample_aspect_ratio; + frame->color_range = link->color_range; return frame; }
As part of achieving our YUVJ-free future, it needs to be possible to pass the colour range property from a decoder context to an encoder context. In the absence of filters, this is obviously trivial, but as soon as filters are introduced, there needs to be a way to pass and preserve the property (or modify it, as some filters will do). Based on existing properties of this type, this change adds a link property and ways to set it from a buffer source and get it from a buffer sink. Signed-off-by: Philip Langdale <philipl@overt.org> --- doc/APIchanges | 5 +++++ libavfilter/avfilter.c | 2 ++ libavfilter/avfilter.h | 2 ++ libavfilter/buffersink.c | 1 + libavfilter/buffersink.h | 1 + libavfilter/buffersrc.c | 23 +++++++++++++++++++++-- libavfilter/buffersrc.h | 5 +++++ libavfilter/version.h | 2 +- libavfilter/video.c | 1 + 9 files changed, 39 insertions(+), 3 deletions(-)