| Message ID | 20190524152354.41425-1-derek.buitenhuis@gmail.com |
|---|---|
| State | Accepted |
| Commit | 80757bed89c361fe2dc56eb61f4cb3b9f9b1aedc |
| Headers | show |
On 5/24/2019 12:23 PM, Derek Buitenhuis wrote: > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > libavcodec/libx265.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c > index 07bca81aef..f56def53d5 100644 > --- a/libavcodec/libx265.c > +++ b/libavcodec/libx265.c > @@ -133,6 +133,14 @@ static av_cold int libx265_encode_init(AVCodecContext *avctx) > return AVERROR(EINVAL); > } > > + Unnecessary empty line. > + ctx->params->vui.bEnableVideoSignalTypePresentFlag = 1; > + > + ctx->params->vui.bEnableVideoFullRangeFlag = avctx->pix_fmt == AV_PIX_FMT_YUVJ420P || > + avctx->pix_fmt == AV_PIX_FMT_YUVJ422P || > + avctx->pix_fmt == AV_PIX_FMT_YUVJ444P || Could we not? The idea is to eventually kill these, so we should at least try to not make them even more widespread... > + avctx->color_range == AVCOL_RANGE_JPEG; > + > if ((avctx->color_primaries <= AVCOL_PRI_SMPTE432 && > avctx->color_primaries != AVCOL_PRI_UNSPECIFIED) || > (avctx->color_trc <= AVCOL_TRC_ARIB_STD_B67 && > @@ -140,7 +148,6 @@ static av_cold int libx265_encode_init(AVCodecContext *avctx) > (avctx->colorspace <= AVCOL_SPC_ICTCP && > avctx->colorspace != AVCOL_SPC_UNSPECIFIED)) { > > - ctx->params->vui.bEnableVideoSignalTypePresentFlag = 1; > ctx->params->vui.bEnableColorDescriptionPresentFlag = 1; > > // x265 validates the parameters internally > @@ -454,8 +461,11 @@ FF_ENABLE_DEPRECATION_WARNINGS > > static const enum AVPixelFormat x265_csp_eight[] = { > AV_PIX_FMT_YUV420P, > + AV_PIX_FMT_YUVJ420P, > AV_PIX_FMT_YUV422P, > + AV_PIX_FMT_YUVJ422P, > AV_PIX_FMT_YUV444P, > + AV_PIX_FMT_YUVJ444P, > AV_PIX_FMT_GBRP, > AV_PIX_FMT_GRAY8, > AV_PIX_FMT_NONE > @@ -463,8 +473,11 @@ static const enum AVPixelFormat x265_csp_eight[] = { > > static const enum AVPixelFormat x265_csp_ten[] = { > AV_PIX_FMT_YUV420P, > + AV_PIX_FMT_YUVJ420P, > AV_PIX_FMT_YUV422P, > + AV_PIX_FMT_YUVJ422P, > AV_PIX_FMT_YUV444P, > + AV_PIX_FMT_YUVJ444P, > AV_PIX_FMT_GBRP, > AV_PIX_FMT_YUV420P10, > AV_PIX_FMT_YUV422P10, > @@ -477,8 +490,11 @@ static const enum AVPixelFormat x265_csp_ten[] = { > > static const enum AVPixelFormat x265_csp_twelve[] = { > AV_PIX_FMT_YUV420P, > + AV_PIX_FMT_YUVJ420P, > AV_PIX_FMT_YUV422P, > + AV_PIX_FMT_YUVJ422P, > AV_PIX_FMT_YUV444P, > + AV_PIX_FMT_YUVJ444P, > AV_PIX_FMT_GBRP, > AV_PIX_FMT_YUV420P10, > AV_PIX_FMT_YUV422P10, >
On 25/05/2019 04:25, James Almer wrote: >> + > > Unnecessary empty line. Fixed. > Could we not? The idea is to eventually kill these, so we should at > least try to not make them even more widespread... As far as I know, they can't be removed, as there isn't a simple replacement for some of their functionality. Has this changed? Either we finally act on their """deprecation""", or we at least have consistent behavior. e.g. libx264.c has almost this exact bit of code, and users would expect libx265.c to behave the same. As far as I'm concerned, until the YUVJ stuff is /actually/ behind deprecation guards, it's deprecated in name only... for years. - Derek
On 5/25/2019 12:50 PM, Derek Buitenhuis wrote: > On 25/05/2019 04:25, James Almer wrote: >>> + >> >> Unnecessary empty line. > > Fixed. > >> Could we not? The idea is to eventually kill these, so we should at >> least try to not make them even more widespread... > > As far as I know, they can't be removed, as there isn't a simple replacement > for some of their functionality. Has this changed? Paul tried to remove them, but as usual a billion obscure use cases and command lines started to behave differently and he gave up. I don't recall if the issue was in swscale or lavfi, but one of those seems to have some code deeply tied to these pseudo formats and no easy solution for it. > > Either we finally act on their """deprecation""", or we at least have > consistent behavior. e.g. libx264.c has almost this exact bit of code, and > users would expect libx265.c to behave the same. libx264 is a very old encoder. New encoders, including this one, vpx and aom, accept only yuv4xxp + color_range value. Is this change really necessary? Why did nobody request it before? > > As far as I'm concerned, until the YUVJ stuff is /actually/ behind deprecation > guards, it's deprecated in name only... for years. > > - Derek > _______________________________________________ > 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 5/25/2019 6:03 PM, James Almer wrote: > On 5/25/2019 12:50 PM, Derek Buitenhuis wrote: >> On 25/05/2019 04:25, James Almer wrote: >>>> + >>> >>> Unnecessary empty line. >> >> Fixed. >> >>> Could we not? The idea is to eventually kill these, so we should at >>> least try to not make them even more widespread... >> >> As far as I know, they can't be removed, as there isn't a simple replacement >> for some of their functionality. Has this changed? > > Paul tried to remove them, but as usual a billion obscure use cases and > command lines started to behave differently and he gave up. I don't > recall if the issue was in swscale or lavfi, but one of those seems to > have some code deeply tied to these pseudo formats and no easy solution > for it. > >> >> Either we finally act on their """deprecation""", or we at least have >> consistent behavior. e.g. libx264.c has almost this exact bit of code, and >> users would expect libx265.c to behave the same. > > libx264 is a very old encoder. New encoders, including this one, vpx and > aom, accept only yuv4xxp + color_range value. Is this change really > necessary? Why did nobody request it before? Ah hell, whatever, just push it. Jeeb pointed out the reason you wrote this patch. Reverting this patch in the future should be trivial anyway. Anyone wants to write libavscale and solve this mess? > >> >> As far as I'm concerned, until the YUVJ stuff is /actually/ behind deprecation >> guards, it's deprecated in name only... for years. >> >> - Derek >> _______________________________________________ >> 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 25/05/2019 22:31, James Almer wrote: > Ah hell, whatever, just push it. Jeeb pointed out the reason you wrote > this patch. Reverting this patch in the future should be trivial anyway. This patch also adds support for checking the AVCOL range. If you object that strongly to it, I'm sure they don't mind switching to -color_range jpeg. That works with x264, etc. too. > Anyone wants to write libavscale and solve this mess? I've been happily using zimg and libplacebo ;). - Derek
On 25/05/2019 22:55, Derek Buitenhuis wrote: > This patch also adds support for checking the AVCOL range. If you object that > strongly to it, I'm sure they don't mind switching to -color_range jpeg. That > works with x264, etc. too. Pushed as-is, as per IRC conversation. - Derek
On Sat, May 25, 2019 at 06:31:14PM -0300, James Almer wrote: > On 5/25/2019 6:03 PM, James Almer wrote: > > On 5/25/2019 12:50 PM, Derek Buitenhuis wrote: > >> On 25/05/2019 04:25, James Almer wrote: > >>>> + > >>> > >>> Unnecessary empty line. > >> > >> Fixed. > >> > >>> Could we not? The idea is to eventually kill these, so we should at > >>> least try to not make them even more widespread... > >> > >> As far as I know, they can't be removed, as there isn't a simple replacement > >> for some of their functionality. Has this changed? > > > > Paul tried to remove them, but as usual a billion obscure use cases and > > command lines started to behave differently and he gave up. I don't > > recall if the issue was in swscale or lavfi, but one of those seems to > > have some code deeply tied to these pseudo formats and no easy solution > > for it. > > > >> > >> Either we finally act on their """deprecation""", or we at least have > >> consistent behavior. e.g. libx264.c has almost this exact bit of code, and > >> users would expect libx265.c to behave the same. > > > > libx264 is a very old encoder. New encoders, including this one, vpx and > > aom, accept only yuv4xxp + color_range value. Is this change really > > necessary? Why did nobody request it before? > > Ah hell, whatever, just push it. Jeeb pointed out the reason you wrote > this patch. Reverting this patch in the future should be trivial anyway. > > Anyone wants to write libavscale and solve this mess? how would rewriting, i presume a libswscale solve a problem between libavfiter and its negotiation of pixel formats when they are split into multiple components instead of an enum? If i remember correctly, the problem was something like that the negotiation system for pixel formats in libafilter is designed for a single value as in an enum. It could be changed to a struct and still treat that as a single value and that might work (with some issues). But when its a series of values which are treated more independantly while they are not actually independant then corner cases start falling appart. I dont even think this is so much an issue with how things are designed but more a fundamental issue of negotiation A list of enums or structs can fully capture what some component supports. 2 seperate lists one of pixel formats and one of color ranges which are independant no longer achieves this Now i may be misremembering parts of this, after all i was IIRC not closely involved in this ... But i think the first step to solve it is to understand what the problem is Thanks [...]
diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c index 07bca81aef..f56def53d5 100644 --- a/libavcodec/libx265.c +++ b/libavcodec/libx265.c @@ -133,6 +133,14 @@ static av_cold int libx265_encode_init(AVCodecContext *avctx) return AVERROR(EINVAL); } + + ctx->params->vui.bEnableVideoSignalTypePresentFlag = 1; + + ctx->params->vui.bEnableVideoFullRangeFlag = avctx->pix_fmt == AV_PIX_FMT_YUVJ420P || + avctx->pix_fmt == AV_PIX_FMT_YUVJ422P || + avctx->pix_fmt == AV_PIX_FMT_YUVJ444P || + avctx->color_range == AVCOL_RANGE_JPEG; + if ((avctx->color_primaries <= AVCOL_PRI_SMPTE432 && avctx->color_primaries != AVCOL_PRI_UNSPECIFIED) || (avctx->color_trc <= AVCOL_TRC_ARIB_STD_B67 && @@ -140,7 +148,6 @@ static av_cold int libx265_encode_init(AVCodecContext *avctx) (avctx->colorspace <= AVCOL_SPC_ICTCP && avctx->colorspace != AVCOL_SPC_UNSPECIFIED)) { - ctx->params->vui.bEnableVideoSignalTypePresentFlag = 1; ctx->params->vui.bEnableColorDescriptionPresentFlag = 1; // x265 validates the parameters internally @@ -454,8 +461,11 @@ FF_ENABLE_DEPRECATION_WARNINGS static const enum AVPixelFormat x265_csp_eight[] = { AV_PIX_FMT_YUV420P, + AV_PIX_FMT_YUVJ420P, AV_PIX_FMT_YUV422P, + AV_PIX_FMT_YUVJ422P, AV_PIX_FMT_YUV444P, + AV_PIX_FMT_YUVJ444P, AV_PIX_FMT_GBRP, AV_PIX_FMT_GRAY8, AV_PIX_FMT_NONE @@ -463,8 +473,11 @@ static const enum AVPixelFormat x265_csp_eight[] = { static const enum AVPixelFormat x265_csp_ten[] = { AV_PIX_FMT_YUV420P, + AV_PIX_FMT_YUVJ420P, AV_PIX_FMT_YUV422P, + AV_PIX_FMT_YUVJ422P, AV_PIX_FMT_YUV444P, + AV_PIX_FMT_YUVJ444P, AV_PIX_FMT_GBRP, AV_PIX_FMT_YUV420P10, AV_PIX_FMT_YUV422P10, @@ -477,8 +490,11 @@ static const enum AVPixelFormat x265_csp_ten[] = { static const enum AVPixelFormat x265_csp_twelve[] = { AV_PIX_FMT_YUV420P, + AV_PIX_FMT_YUVJ420P, AV_PIX_FMT_YUV422P, + AV_PIX_FMT_YUVJ422P, AV_PIX_FMT_YUV444P, + AV_PIX_FMT_YUVJ444P, AV_PIX_FMT_GBRP, AV_PIX_FMT_YUV420P10, AV_PIX_FMT_YUV422P10,
Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> --- libavcodec/libx265.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)