ffmpeginteropx / FFmpegInteropX

FFmpeg decoding library for Windows 10 UWP and WinUI 3 Apps
Apache License 2.0
211 stars 53 forks source link

Update for ffmpeg 5.0 #251

Closed lukasf closed 2 years ago

lukasf commented 2 years ago

The PR updates to FFmpeg 5.0. It also adds HDR support!

MouriNaruto commented 2 years ago

@lukasf

You said these in https://github.com/ffmpeginteropx/FFmpegInteropX/pull/250#issuecomment-1022537133, I will reply in this PR.

Oh man, it has only been two weeks since we pushed the FFmpeg 4.4 release. And now FFmpeg 5.0 is out? ^^

The implementation in FillLinesAndBuffer follows what av_image_alloc does internally, which also calls av_image_fill_pointers with a zero buffer to get the buffer size, instead of using av_image_get_buffer_size. I am not sure why it does not use av_image_get_buffer_size, but I'd rather follow what FFmpeg does internally, instead of using a different API.

The "hacky" part was a micro optimization to avoid a second run of size calculations. av_image_alloc just does a second call to av_image_fill_pointers, after it has the size from first call and allocated the memory. I think we should do that as well. It is not an expensive call anyways.

Still, thank you for your work! Of course it does not make sense to solve only half the issues with FFmpeg 5.0. If we want to move to FFmpeg 5.0, we need to solve all issues. Since I cannot work on your branch, I have created a new PR #251 . By the way, you have Collaborator status in this repo. So you can create branches directly in FFmpegInteropX, and we can all work on the same PR.

I have confirmed the implementation of FillLinesAndBuffer follows what av_image_alloc does internally in FFmpeg 4.0.

https://github.com/FFmpeg/FFmpeg/blob/7052da6837285528c723a61d3d16b62ed72c6044/libavutil/imgutils.c#L192

    if ((ret = av_image_fill_pointers(pointers, pix_fmt, h, NULL, linesizes)) < 0)
        return ret;
    buf = av_malloc(ret + align);
    if (!buf)
        return AVERROR(ENOMEM);
    if ((ret = av_image_fill_pointers(pointers, pix_fmt, h, buf, linesizes)) < 0) {
        av_free(buf);
        return ret;
    }

But at least in FFmpeg 4.4, things has been changed.

https://github.com/FFmpeg/FFmpeg/blob/3e539d11e4a78d44e8d95cef1d67d67e00258e31/libavutil/imgutils.c#L216

    if ((ret = av_image_fill_plane_sizes(sizes, pix_fmt, h, linesizes1)) < 0)
        return ret;
    total_size = align;
    for (i = 0; i < 4; i++) {
        if (total_size > SIZE_MAX - sizes[i])
            return AVERROR(EINVAL);
        total_size += sizes[i];
    }
    buf = av_malloc(total_size);
    if (!buf)
        return AVERROR(ENOMEM);
    if ((ret = av_image_fill_pointers(pointers, pix_fmt, h, buf, linesizes)) < 0) {
        av_free(buf);
        return ret;
    }

Here is the reason of the change: https://github.com/FFmpeg/FFmpeg/commit/3a8e9271765a07e509655346ef88a28578cbff47.

Maybe we can follow that.

Kenji Mouri

lukasf commented 2 years ago

Agreed @MouriNaruto, thanks for the hint. I have pushed a new changeset to align with ffmpeg 5 implementation.

brabebhin commented 2 years ago

Awesome work guys! I have tested a few files and it seems to be all fine. But I suggest we give it some more testing.

lukasf commented 2 years ago

I have tested a lot of files now, all seems to work well.

I found a few files which had broken P-frames with HW acceleration enabled. It is not related to this PR, it happened with older builds as well. So now I do not enable multi threading if GPU decoding is used.

brabebhin commented 2 years ago

Well, technically, directx 11, which we are using for decoding on the GPU, does not really support multi threading. It may very well be that turning it on may cause problems, like we have observed some time ago.

lukasf commented 2 years ago

I am pretty sure that FFmpeg does proper locking for its multi threading. But trying to process images out-of-order when using GPU decoding does not make sense and will cause trouble. There is a reason why it is usually prevented internally in FFmpeg decoders...

lukasf commented 2 years ago

Merged!