nyanmisaka / ffmpeg-rockchip

FFmpeg with async and zero-copy Rockchip MPP & RGA support
Other
326 stars 48 forks source link

Introduce AVRKMPPDRMFrameDescriptor #2

Closed hbiyik closed 6 months ago

hbiyik commented 6 months ago

This is exactly like AVDRMFrameDescriptor but attionally it holds MppBuffer instance used on each index. With that, AVDRMObjectDescriptor can be kept as is, and no need to modify the existing API.

MajorChanges: Introduce AVRKMPPDRMFrameDescriptor which is inheriting AVDRMFrameDescriptor. Note: if ptr is accepted to be a member off AVDRMObjectDescriptor, then this change would not be necessary. Open for discussion.

Adapt hwcontext_rkmpp.c from AVDRMFrameDescriptor->AVRKMPPDRMFrameDescriptor. (Decoder / encoder and rga scalers are still using AVDRMFrameDescriptor and i think this is fine for future compatibility.)

Remove rkmpp_buffer_free, and use only rkmpp_free_drm_frame_descriptor for both desc and mpp_buf clean up. With this, there is no need to reference tge mppbuffer to drmdescriptor.

Not througly tested yet.

nyanmisaka commented 6 months ago

Cleaner but this conflicts with the description of the AV_PIX_FMT_DRM_PRIME format in pixfmt.h.

    /**
     * DRM-managed buffers exposed through PRIME buffer sharing.
     *
     * data[0] points to an AVDRMFrameDescriptor.
     */
    AV_PIX_FMT_DRM_PRIME,

https://github.com/FFmpeg/FFmpeg/blob/b61733f61ff2f61b1208fb661e278f704680d6a6/libavutil/pixfmt.h#L348

Doing so is basically similar to migrating to a new format AV_PIX_FMT_RKMPP.

hbiyik commented 6 months ago

I did not get it fully may be you elaborate if im mistaken.

Idea was to keep rkmpp encoder/decoder/rgascalers compatible to AV_PIX_FMT_DRM_PRIME and AVFrameDescriptor

the AVRKMPPDRMFrameDescriptor structs are castable to AVFrameDescriptor structs and as longs as their scope is limited in the hwcontext_rkmpp* and not exposed to somewhere else, then AV_PIX_FMT_DRM_PRIME should be fine?

Yet i agree this is little overkill just to provide ptr to drmdescriptor.

I think the problem space changes in case this fork will ever be mainlined or not.

If it is going to be mainline, then it is just the cleanest to add ptr to the objectdescriptor. Since the api change will be globally in each system that will cause no problem.

If no mainline is planned, the breaking api will cause everyshared library user to be rebuilt, which is a problem and i believe will require an invention like above.

nyanmisaka commented 6 months ago

I see. It makes sense then.

hbiyik commented 6 months ago

i think there is a problem here:

this will set is_rga2_used to true whenever a matching input/output format is detected and this will fail afterwards in several different ways, ie: when output mode is set to afbc, or core is set to some specific rga3 core..

nyanmisaka commented 6 months ago

i think there is a problem here:

this will set is_rga2_used to true whenever a matching input/output format is detected and this will fail afterwards in several different ways, ie: when output mode is set to afbc, or core is set to some specific rga3 core..

Yes, this is expected behavior, when manual settings and hardware capabilities do not match it will simply fail and prompt the reason. TBH this annoying stuff should be handled by librga, but currently librga doesn't even have a reliable API to query hardware capabilities.

Btw user/ffmpeg should not know the existence of RGA2 and RGA3, but get the supported pixel formats, resolutions, etc. by querying the API. This is what the driver should do.

The VAAPI frontend is able to meet all the needs here, rather than hardcoding them in ffmpeg like this.

hbiyik commented 6 months ago

ok, rga is always a problem and saviour at the same time, love and hate realtion.

Btw, contentwise this PR is good to merge, i have tested thorughly. How do you want to proceed? May be i can seperate this in to 2 different commits:

1) Remove rkmpp_buffer_free, and use only rkmpp_free_drm_frame_descriptor 2) AVRKMPPDRMFrameDescriptor

Or is it not necessary, how do you like to go on?

nyanmisaka commented 6 months ago

I also tested it and will merge it later.

nyanmisaka commented 6 months ago

Manually merged in 50cc62684ab9b062cecf7db2d3d5e0b8a137c49f