| Message ID | 20230617081108.10051-1-thilo.borgmann@mail.de |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v1,1/2] lavc/vp9: set yuvj pixel format for full range decode | 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 |
NAK, there is color_range that should not be ignored. Fix finally scale filter.
Am 17.06.23 um 13:40 schrieb Paul B Mahol: > NAK, there is color_range that should not be ignored. In patch 1/2 color_range is used in the condition for the switch. if (avctx->color_range == AVCOL_RANGE_JPEG) { Or I don't understand. What shall be done with color_range? > Fix finally scale filter. Hmm? Where what who? -Thilo
On Sat, Jun 17, 2023 at 1:47 PM Thilo Borgmann <thilo.borgmann@mail.de> wrote: > Am 17.06.23 um 13:40 schrieb Paul B Mahol: > > NAK, there is color_range that should not be ignored. > > In patch 1/2 color_range is used in the condition for the switch. > > if (avctx->color_range == AVCOL_RANGE_JPEG) { > > Or I don't understand. What shall be done with color_range? > Actually used, and all YUVJ uses finally removed from codebase. > > > > Fix finally scale filter. > > Hmm? Where what who? > Perhaps just frame->color_range needs to be set? > > -Thilo > > _______________________________________________ > 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". >
Am 17.06.23 um 13:53 schrieb Paul B Mahol: > On Sat, Jun 17, 2023 at 1:47 PM Thilo Borgmann <thilo.borgmann@mail.de> > wrote: > >> Am 17.06.23 um 13:40 schrieb Paul B Mahol: >>> NAK, there is color_range that should not be ignored. >> >> In patch 1/2 color_range is used in the condition for the switch. >> >> if (avctx->color_range == AVCOL_RANGE_JPEG) { >> >> Or I don't understand. What shall be done with color_range? >> > > Actually used, and all YUVJ uses finally removed from codebase. Complete removal shall be done, yes. (Hopefully very soon actually) However, as long as we didn't and nobody can really guarantee anything about when it will be, having this is better than the current state. >>> Fix finally scale filter. >> >> Hmm? Where what who? >> > > Perhaps just frame->color_range needs to be set? Where does a scale filter correlate with this? Do you mean setting this in 2/2? I can't see what you want me to do here... -Thilo
On 6/17/23 04:11, Thilo Borgmann wrote: > While the yuvj pixel formats are deprecated lots of code still relies > on them to be set. Without setting a yuvj420p pixel format VP9 > decoding ends up incorrectly due to auto conversion. > I oppose this on principle. If there's code that relies on YUVJ being set, then *that code* needs to be changed so it respects the AVFrame->color_range field. Which code is working improperly with this? - Leo Izen
Am 17.06.23 um 16:02 schrieb Leo Izen: > On 6/17/23 04:11, Thilo Borgmann wrote: >> While the yuvj pixel formats are deprecated lots of code still relies >> on them to be set. Without setting a yuvj420p pixel format VP9 >> decoding ends up incorrectly due to auto conversion. >> > > I oppose this on principle. If there's code that relies on YUVJ being set, then *that code* needs to be changed so it respects the AVFrame->color_range field. Which code is working improperly with this? I don't like adding YUVJ stuff either. If I do ./ffmpeg -i full-range-in.mp4 -c:v libvpx-vp9 -lossless 1 lossless-out.mp4 and then comparing the frames, they are not equal. E.g. by ./ffmpeg -i full-range-in.mp4 -i lossless-out.mp4 -filter_complex ssim -f crc - they are not 1.0 in ssim terms. I agree, maybe the same effect can be fixed somewhere else and if so it could effect more codecs than VP9. You (or anyone) have an idea? Thanks, Thilo
On 6/17/23 10:26, Thilo Borgmann wrote: > Am 17.06.23 um 16:02 schrieb Leo Izen: >> On 6/17/23 04:11, Thilo Borgmann wrote: >>> While the yuvj pixel formats are deprecated lots of code still relies >>> on them to be set. Without setting a yuvj420p pixel format VP9 >>> decoding ends up incorrectly due to auto conversion. >>> >> >> I oppose this on principle. If there's code that relies on YUVJ being >> set, then *that code* needs to be changed so it respects the >> AVFrame->color_range field. Which code is working improperly with this? > > I don't like adding YUVJ stuff either. If I do > > ./ffmpeg -i full-range-in.mp4 -c:v libvpx-vp9 -lossless 1 > lossless-out.mp4 In this case, it looks like the libvpx-vp9 wrapper is not respecting the AVFrame->color_range tag, or the vp9 decoder is not setting it. In either case, I don't mind the patch that allows the vp9 encoder to handle YUVJ formats correctly if they are passed, but it needs to also respect the color_range tag if it's set to AVCOL_RANGE_JPEG. The ffvp9 decoder should, in my opinion, produce yuv420p frames with the flag set, and then the libvpx-vp9 encoder wrapper should respect the flag, and treat yuv420p with RANGE_JPEG as though it were passed yuvj420p, and do the same thing. In theory, if both of these are implemented, then the command above should work. However, I admit I'm not sure how much work it is relative to the patch you just sent. When I find some time I could take a look at it as well. - Leo Izen
On 6/17/23 10:26, Thilo Borgmann wrote: > Am 17.06.23 um 16:02 schrieb Leo Izen: >> On 6/17/23 04:11, Thilo Borgmann wrote: >>> While the yuvj pixel formats are deprecated lots of code still relies >>> on them to be set. Without setting a yuvj420p pixel format VP9 >>> decoding ends up incorrectly due to auto conversion. >>> >> >> I oppose this on principle. If there's code that relies on YUVJ being >> set, then *that code* needs to be changed so it respects the >> AVFrame->color_range field. Which code is working improperly with this? > > I don't like adding YUVJ stuff either. If I do > > ./ffmpeg -i full-range-in.mp4 -c:v libvpx-vp9 -lossless 1 > lossless-out.mp4 > > and then comparing the frames, they are not equal. E.g. by > > ./ffmpeg -i full-range-in.mp4 -i lossless-out.mp4 -filter_complex ssim > -f crc - > > they are not 1.0 in ssim terms. > Are you sure? I just tested a sample and found that I got exactly 1.0 in ssim terms. Do you have a link to a sample for which this fails? - Leo Izen
Am 18.06.23 um 23:21 schrieb Leo Izen: > On 6/17/23 10:26, Thilo Borgmann wrote: >> Am 17.06.23 um 16:02 schrieb Leo Izen: >>> On 6/17/23 04:11, Thilo Borgmann wrote: >>>> While the yuvj pixel formats are deprecated lots of code still relies >>>> on them to be set. Without setting a yuvj420p pixel format VP9 >>>> decoding ends up incorrectly due to auto conversion. >>>> >>> >>> I oppose this on principle. If there's code that relies on YUVJ being set, >>> then *that code* needs to be changed so it respects the AVFrame->color_range >>> field. Which code is working improperly with this? >> >> I don't like adding YUVJ stuff either. If I do >> >> ./ffmpeg -i full-range-in.mp4 -c:v libvpx-vp9 -lossless 1 lossless-out.mp4 >> >> and then comparing the frames, they are not equal. E.g. by >> >> ./ffmpeg -i full-range-in.mp4 -i lossless-out.mp4 -filter_complex ssim -f crc - >> >> they are not 1.0 in ssim terms. >> > > Are you sure? I just tested a sample and found that I got exactly 1.0 in ssim > terms. Do you have a link to a sample for which this fails? IIRC I had the same impression when testing, I think I mixed up patched and unpatched ffmpeg builds. Anyway, happy you retest, I used ./ffmpeg -f lavfi -i testsrc=duration=10:size=1280x720:rate=30 -pix_fmt yuvj420p -color_range pc full-range-in.mp4 to generate my input sample. I cannot test myself again until Thursday, my Laptop is not equipped with libvpx. Thank you! -Thilo
On 6/19/23 04:14, Thilo Borgmann wrote: > Am 18.06.23 um 23:21 schrieb Leo Izen: >> On 6/17/23 10:26, Thilo Borgmann wrote: >>> Am 17.06.23 um 16:02 schrieb Leo Izen: >>>> On 6/17/23 04:11, Thilo Borgmann wrote: >>>>> While the yuvj pixel formats are deprecated lots of code still relies >>>>> on them to be set. Without setting a yuvj420p pixel format VP9 >>>>> decoding ends up incorrectly due to auto conversion. >>>>> >>>> >>>> I oppose this on principle. If there's code that relies on YUVJ >>>> being set, then *that code* needs to be changed so it respects the >>>> AVFrame->color_range field. Which code is working improperly with this? >>> >>> I don't like adding YUVJ stuff either. If I do >>> >>> ./ffmpeg -i full-range-in.mp4 -c:v libvpx-vp9 -lossless 1 >>> lossless-out.mp4 >>> >>> and then comparing the frames, they are not equal. E.g. by >>> >>> ./ffmpeg -i full-range-in.mp4 -i lossless-out.mp4 -filter_complex >>> ssim -f crc - >>> >>> they are not 1.0 in ssim terms. >>> >> >> Are you sure? I just tested a sample and found that I got exactly 1.0 >> in ssim terms. Do you have a link to a sample for which this fails? > > IIRC I had the same impression when testing, I think I mixed up patched > and unpatched ffmpeg builds. > > Anyway, happy you retest, I used > > ./ffmpeg -f lavfi -i testsrc=duration=10:size=1280x720:rate=30 -pix_fmt > yuvj420p -color_range pc full-range-in.mp4 > > to generate my input sample. I cannot test myself again until Thursday, > my Laptop is not equipped with libvpx. > I used: ./ffmpeg -i input.mkv -vf libplacebo=format=yuv420p:range=pc -c ffv1 full-range-in.mkv I wonder if using yuv420p with pc range changes the results. Running ffmpeg -i full-range-in.mkv by itself reports ffv1, yuv420p(pc, progressive) as its format. - Leo Izen
diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c index 4f704ec0dd..e7be755fe6 100644 --- a/libavcodec/vp9.c +++ b/libavcodec/vp9.c @@ -202,6 +202,7 @@ static int update_size(AVCodecContext *avctx, int w, int h) switch (s->pix_fmt) { case AV_PIX_FMT_YUV420P: + case AV_PIX_FMT_YUVJ420P: case AV_PIX_FMT_YUV420P10: #if CONFIG_VP9_DXVA2_HWACCEL *fmtp++ = AV_PIX_FMT_DXVA2_VLD; @@ -509,6 +510,25 @@ static int read_colorspace_details(AVCodecContext *avctx) s->ss_h = s->ss_v = 1; s->pix_fmt = pix_fmt_for_ss[bits][1][1]; } + + if (avctx->color_range == AVCOL_RANGE_JPEG) { + switch (s->pix_fmt) { + case AV_PIX_FMT_YUV444P: + s->pix_fmt = AV_PIX_FMT_YUVJ444P; + break; + case AV_PIX_FMT_YUV422P: + s->pix_fmt = AV_PIX_FMT_YUVJ422P; + break; + case AV_PIX_FMT_YUV440P: + s->pix_fmt = AV_PIX_FMT_YUVJ440P; + break; + case AV_PIX_FMT_YUV420P: + s->pix_fmt = AV_PIX_FMT_YUVJ420P; + break; + default: + break; + } + } } return 0;