mpv-player / mpv

🎥 Command line video player
https://mpv.io
Other
27.82k stars 2.87k forks source link

DRMPrime NV16 support #12491

Closed hbiyik closed 11 months ago

hbiyik commented 11 months ago

Expected behavior of the wanted feature

I have created an FFmpeg fork which provides encoder and decoder support with the latest mpp for rockchip devices located at https://github.com/hbiyik/FFmpeg.

I want to enable NV16 support first to egl import in mpp so that i can play NV16 frames provided by the decoder smoothly in the mpv. If i can achieve that then i will take my chances at NV15 but i am not sure EGL will like this plane format. It is rockchip specific weird format.

I have teached myself the image formats quiet a bit during this work, but when it comes to rendering i am super noob. I have patched the mpv as below so it wont complain, but when NV16 drm planes are played, the picture is green. I think somewhere the picture planes must be BLITted to RGBA to import in EGL but dont know, how to proceed.

Is it achievable? Where should i look at to troubleshoot this green picture issue?

PS: NV12 works just fine. PS: I am playin with ffplay --hwdec=rkmpp --vo=gpu somefile.mp4

Alternative behavior of the wanted feature

Log file

diff --git a/video/out/hwdec/dmabuf_interop_gl.c b/video/out/hwdec/dmabuf_interop_gl.c
index bd33474..e7fb103 100644
--- a/video/out/hwdec/dmabuf_interop_gl.c
+++ b/video/out/hwdec/dmabuf_interop_gl.c
@@ -176,6 +176,7 @@
         if (p_mapper->desc.layers[i].nb_planes > 1) {
             switch (p_mapper->desc.layers[i].format) {
             case DRM_FORMAT_NV12:
+            case DRM_FORMAT_NV16:
                 format[0] = DRM_FORMAT_R8;
                 format[1] = DRM_FORMAT_GR88;
                 break;
diff --git a/video/out/hwdec/hwdec_drmprime.c b/video/out/hwdec/hwdec_drmprime.c
index 5051207..7b0239a 100644
--- a/video/out/hwdec/hwdec_drmprime.c
+++ b/video/out/hwdec/hwdec_drmprime.c
@@ -116,6 +116,7 @@
      */
     int num_formats = 0;
     MP_TARRAY_APPEND(p, p->formats, num_formats, IMGFMT_NV12);
+    MP_TARRAY_APPEND(p, p->formats, num_formats, IMGFMT_AVPIXFMT_START + AV_PIX_FMT_NV16);
     MP_TARRAY_APPEND(p, p->formats, num_formats, IMGFMT_420P);
     MP_TARRAY_APPEND(p, p->formats, num_formats, 0); // terminate it
CounterPillow commented 11 months ago

I have created an FFmpeg fork

stop

upstream your shit

hbiyik commented 11 months ago

would love to, but ffmpeg sued rockchip so not likely, nobody will maintain it.

CounterPillow commented 11 months ago

but ffmpeg sued rockchip

what?

hbiyik commented 11 months ago

ok, this is getting quite side-tracked but, Rockchip, the manufacturer of SOCs being used lots of SBCs and STBs has been sued by FFMpeg due to reasons that i do not know, speculatively due to their BSP business having FFMpeg in binary form in some of their customers, but really they also contributed to FFMpeg as well, so intention-wise i don't know they are extorting anything.

FFmpeg in the upstream already has rockchip device support through mpp, which is a user space library provided by Rockchip to acccess their VPU cores https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/rkmppdec.c. So it was fine in between them in past.

Since there is this legal issue in between, rockchip part of ffmpeg has not been properly touched since 2017 and it is quite potato, even the mpp interface used is very much outdated.

Therefore i would upstream my code, but, not sure, ffmpeg would be interested. 2nd, i am also not sure how my code is compliant with the upstream conventions, and i constantly improve it, because dealing with several different VPU cores and Rastering Chips (RGA) is quite tricky. So may be when i make sure the quality in my code is good enough, i can propose to merge mainline, but i doubt any one will take this up FFMpeg because the future maintenance will be a burden. I have already rebased to ffmpeg 4.x, 5.x and 6.x and refactored 3 times, so no problem.

Besides all of those, do you think this is relevant to the actual question asked?

EDIT: Also, mainline linux is taking the approach of v4l2m2m for rockchip decoders (hantro variants), so all mpp and rockchip ffmpeg things might be irrelevant in future, but NV16 support will still be required for DRMPrime even with v4l2m2m..

CounterPillow commented 11 months ago

has been sued by FFMpeg due to reasons that i do not know,

I doubt FFmpeg sued them considering individual contributors own the rights to their code, not FFmpeg as a legal entity if it even exists.

Besides all of those, do you think this is relevant to the actual question asked?

Yes, because there's currently like 3 different FFmpeg forks for embedded stuff floating around and all of them want to occasionally add new image formats to mpv. Whether that's appropriate is for mpv's maintainers to decide, but the reluctance to ever contribute anything upstream that embedded developers have is most likely not helping your case.

philipl commented 11 months ago

Right, FFmpeg is not an entity that can sue anyone. The only relevant legal entities are videolan and fflabs, neither of which have engaged with rockchip to the best of my knowledge. Individuals can, of course, have acted on their own, but I'm not aware of it. Equally, I also recall some discussion of rockchip distribution binaries without properly complying with the LGPL requirements.

Separately, despite the fork-happy nature of the downstream SoC community (the rockckip community is no better or worse than rpi or anyone else), one thing that is clear is that they are focused on mainline kernels and supporting accelerating via v4l2, and no one is spending any time on rkmpp. This means a couple of things:

Assuming the kernel side support for NV16 was done correctly, and the ffmpeg v4l2-request decoder part was done correctly, you should get drm prime frames in NV16 with the correct ffmpeg pixfmt set on them. If that happens, your patch above should be sufficient.

Chroma problems (as you suggest is happening) will be due to incorrect identification of the pixfmt, so the data is incorrectly interpreted. Or, there is something wrong in the kernel or ffmpeg part.

I'll wish you good luck. The mpv developers have some access to rockchip hardware, which we used to work on the current support, but no one is going to spend any time looking at anything based on rkmpp.

hbiyik commented 11 months ago

Fair, i understand your concerns and agree most of them, and i do not know and do not want to know the legal fun/drama that Rockchip is having with the OSS community, but i can see their efforts to make it right since they are also working with collabora on several aspects to mainline linux. But please, i am just a random guy on the internet with an SBC in his hand to play with.

your patch above should be sufficient.

ok understood, thats very helpfull, i need to check if something is wrong on the AVFrame side.

But i think, NV16 (so called YCbCR 422 SP or YUV422SP), is something very common and not rockchip specific. It is the half chroma sampled version of NV12 where it is quarter chroma sampled. When the applications require color preciseness, 420 subsampling might not be enough for applications like video-wall displays in the industry. NV15 on the otherhand is different story. So lets limit the discussion with NV16.

I can verify my ffmpeg implementation with direct plane write method to DRM for comparison, so when v4l2m2m or vl42requests is stable enough, NV16 can be used mpv.

When i make NV16 work, would you be interested in a PR?

philipl commented 11 months ago

If you can get NV16 working with https://github.com/jernejsk/ffmpeg/tree/v4l2-request-n6.0, and your patch looks like the one you had above, then sure, I'd accept it.

hbiyik commented 11 months ago

ok thanks for your support, i can ask some people with rk3399 chipset, they should be able to get nv16 frames v4l2-requests with mainline kernel for extra verification.

hbiyik commented 11 months ago

ok tested to be working and send the PR at https://github.com/mpv-player/mpv/pull/12513

My green screen issue was related to file dimensions being bigger than the screen resolution. It also happens in a NV12 format, i will try to figure it out, in case i find somethign related to mpv i can raise another bug ticket. Closing the ticket.

Thanks again.