Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce AVRKMPPDRMFrameDescriptor #2

Closed
wants to merge 1 commit into from

Conversation

hbiyik
Copy link

@hbiyik hbiyik commented Jan 1, 2024

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.

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.
@nyanmisaka
Copy link
Owner

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
Copy link
Author

hbiyik commented Jan 2, 2024

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
Copy link
Owner

I see. It makes sense then.

@hbiyik
Copy link
Author

hbiyik commented Jan 2, 2024

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
Copy link
Owner

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
Copy link
Author

hbiyik commented Jan 3, 2024

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
Copy link
Owner

I also tested it and will merge it later.

@nyanmisaka
Copy link
Owner

Manually merged in 50cc626

@nyanmisaka nyanmisaka closed this Jan 3, 2024
@hbiyik hbiyik deleted the ffmpeg-rockchip-keepapi branch January 19, 2024 15:04
nyanmisaka pushed a commit that referenced this pull request Apr 17, 2024
In close_output(), a dummy frame is created with format NONE passed
to enc_open(), which isn't prepared for it. The NULL pointer
dereference happened at
av_pix_fmt_desc_get(enc_ctx->pix_fmt)->comp[0].depth.

When fgt.graph is NULL, skip fg_output_frame() since there is
nothing to output.

frame #0: 0x0000005555bc34a4 ffmpeg_g`enc_open(opaque=0xb400007efe2db690, frame=0xb400007efe2d9f70) at ffmpeg_enc.c:235:44
frame #1: 0x0000005555bef250 ffmpeg_g`enc_open(sch=0xb400007dde2d4090, enc=0xb400007e4e2daad0, frame=0xb400007efe2d9f70) at ffmpeg_sched.c:1462:11
frame #2: 0x0000005555bee094 ffmpeg_g`send_to_enc(sch=0xb400007dde2d4090, enc=0xb400007e4e2daad0, frame=0xb400007efe2d9f70) at ffmpeg_sched.c:1571:19
frame #3: 0x0000005555bee01c ffmpeg_g`sch_filter_send(sch=0xb400007dde2d4090, fg_idx=0, out_idx=0, frame=0xb400007efe2d9f70) at ffmpeg_sched.c:2154:12
frame #4: 0x0000005555bcf124 ffmpeg_g`close_output(ofp=0xb400007e4e2d85b0, fgt=0x0000007d1790eb08) at ffmpeg_filter.c:2225:15
frame #5: 0x0000005555bcb000 ffmpeg_g`fg_output_frame(ofp=0xb400007e4e2d85b0, fgt=0x0000007d1790eb08, frame=0x0000000000000000) at ffmpeg_filter.c:2317:16
frame #6: 0x0000005555bc7e48 ffmpeg_g`filter_thread(arg=0xb400007eae2ce7a0) at ffmpeg_filter.c:2836:15
frame #7: 0x0000005555bee568 ffmpeg_g`task_wrapper(arg=0xb400007d8e2db478) at ffmpeg_sched.c:2200:21

Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
nyanmisaka pushed a commit that referenced this pull request Apr 17, 2024
The formal title of the muxer according to the specification
is "RCWT (Raw Captions With Time)", so canonize this
in the long name of the codec and docs.

In the documentation section, point #2 was wrong: ccextractor
extracts the Closed Captions data and stores normalized bits
similarly to this muxer.

Signed-off-by: Marth64 <marth64@proxyid.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants