[FFmpeg-devel] [PATCH] Add support for RockChip Media Process Platform

[wm4]

On Tue, 13 Jun 2017 11:17:35 +0100
Mark Thompson <sw at jkqxz.net> wrote:

> > +vp8_rkmpp_hwaccel_deps="rkmpp"
> >  vp9_d3d11va_hwaccel_deps="d3d11va DXVA_PicParams_VP9"
> >  vp9_d3d11va_hwaccel_select="vp9_decoder"
> >  vp9_dxva2_hwaccel_deps="dxva2 DXVA_PicParams_VP9"  
> 
> Why do these hwaccels exist?  They don't appear to do anything, since you only have hardware surface output anyway.

Well I guess you might have a point - as long as get_format isn't
actually involved, they're technically not needed. And we've apparently
always had hw decoders which can output both sw and hw surfaces, so
this never came up (probably).


I still think there's no harm in adding them.

> > ...
> > diff --git a/libavcodec/drmprime.h b/libavcodec/drmprime.h
> > new file mode 100644
> > index 0000000..44816db
> > --- /dev/null
> > +++ b/libavcodec/drmprime.h
> > @@ -0,0 +1,17 @@
> > +#ifndef AVCODEC_DRMPRIME_H
> > +#define AVCODEC_DRMPRIME_H
> > +
> > +#include <stdint.h>
> > +
> > +#define AV_DRMPRIME_NUM_PLANES  4   // maximum number of planes
> > +
> > +typedef struct av_drmprime {
> > +
> > +    int strides[AV_DRMPRIME_NUM_PLANES];    // stride in byte of the according plane
> > +    int offsets[AV_DRMPRIME_NUM_PLANES];    // offset from start in byte of the according plane
> > +    int fds[AV_DRMPRIME_NUM_PLANES];        // file descriptor for the plane, (0) if unused.
> > +    uint32_t format;                        // FOURC_CC drm format for the image
> > +
> > +} av_drmprime;
> > +
> > +#endif // AVCODEC_DRMPRIME_H  
> 
> I'm not sure that a structure like this should be baked into the API/ABI now as a generic thing.  Are you confident that it is sufficient to represent any picture in a DRM object which might be used in future?  (Including things like tiling modes, field splitting, other craziness that GPU makers dream up?)

This includes the DRM FourCC, which should take most craziness into
account. The RockChip decoder itself does something pretty crazy (10
bit _packed_ _planar_ format, wtf?), and it sort of fits in.

I had similar concerns, and I think I basically requested that all
standard eglCreateImageKHR() parameters for this to be included in this
struct (which was done).

It's true though that drivers could invent additional craziness as
additional EGL attributes, or by adding new DRM API calls.

It boils down to how the DRM kernel API handles these things, which I
don't know. (Not like kernel devs would write docs - they'd possibly be
forced to create something consistent!)

Longchair replied with the EGL mapping as use case, but you can also
map it as DRM framebuffer, which appears to happen around here:

https://github.com/LongChair/mpv/blob/rockchip/video/out/opengl/hwdec_drmprime_drm.c#L102

> Can you explain the cases you are using this in?  Are you intending to make other components with input / output in this format?
> 
> With just this decoder in isolation, it would seem preferable to me to make a structure specific to the API for now (or use the existing one - is MppFrame sufficient?).  If later there are multiple implementations and need for a common structure like this then it will be clearer what the requirements are.  Compare VAAPI and QSV (possibly also CUDA, CUVID, VDPAU?) - they can expose DRM objects, but don't currently in the ffmpeg API because any consumers which want them in that form generally prefer the wrapping object including the relevant metadata anyway.

So output those as MppFrames, and then map them as DRM frames? We'd
have the same issues. Also, the API would be harder to use. The
assumption is that native MppFrames are pretty useless anyway, other
than for using them with the MPP decode API.

> Also, zero is a valid file descriptor.

Technically yes, not sure if we care about that case.