On Mon, 30 Jan 2017 10:58:07 +0000 Mark Thompson <[email protected]> wrote:
> On 30/01/17 09:32, wm4 wrote: > > On Sun, 29 Jan 2017 19:57:13 +0000 > > Mark Thompson <[email protected]> wrote: > >> constraints = > >> - av_hwdevice_get_hwframe_constraints(ctx->frames->device_ref, > >> + av_hwdevice_get_hwframe_constraints(avctx->hw_device_ctx ? > >> + avctx->hw_device_ctx : > >> + ctx->frames->device_ref, > >> hwconfig); > >> if (!constraints) { > >> - // Ignore. > > > > Is it required now? Does this have any consequences that could arise in > > practice? > > We now require it in order to get the surface formats. It fails on OOM or if > the query function returns failure, so I think depending on it is reasonable. > (Though if you want the VDPAU wrapper to continue to work that might require > more hackery...) Should be fine then. Regarding the vdpau wrapper, I have a patch that makes it working with Libav git master, but I guess it's better to drop it. > >> + // Find the first format in the list which matches the expected > >> + // bit depth and subsampling. If none are found (this can happen > >> + // when 10-bit streams are decoded to 8-bit surfaces, for example) > >> + // then just take the first format on the list. > >> + ctx->surface_format = constraints->valid_sw_formats[0]; > >> + sw_desc = av_pix_fmt_desc_get(avctx->sw_pix_fmt); > >> + for (i = 0; constraints->valid_sw_formats[i] != AV_PIX_FMT_NONE; i++) > >> { > >> + desc = av_pix_fmt_desc_get(constraints->valid_sw_formats[i]); > >> + if (desc->nb_components != sw_desc->nb_components || > >> + desc->log2_chroma_w != sw_desc->log2_chroma_w || > >> + desc->log2_chroma_h != sw_desc->log2_chroma_h) > >> + continue; > >> + for (j = 0; j < desc->nb_components; j++) { > >> + if (desc->comp[j].depth != sw_desc->comp[j].depth) > >> + break; > >> } > >> + if (j < desc->nb_components) > >> + continue; > >> + ctx->surface_format = constraints->valid_sw_formats[i]; > >> + break; > >> + } > > > > A bit questionable. Might be better to list the formats explicitly, but > > if it works I don't mind either. > > > > Is this even needed? Intuitively, I'd totally expect that the > > av_hwdevice_get_hwframe_constraints() API returns the best format > > for the given config first, or so. Or can it happen that a decoder > > for example supports 10 bit decoding with 8 bit surfaces? Or maybe the > > config just isn't specific enough to include these details? > > Current Mesa decodes H.265 Main10 into 8-bit surfaces, and this code is > trying to satisfy that case and also be ready to pick 10-bit surfaces when > those are available. Well, what Mesa does sounds really questionable... Does it just decode to garbage? Maybe it would be better to error out if no format can be found (instead of using the first)? > (I was very much trying to avoid listing the formats explicitly.) IMO wouldn't be so wrong, since this code deals with highly specific codc and vaapi details anyway. But if you feel better about the code you posted, I don't see anything wrong with it either. Though I'm still slightly worried about future surprises this could possibly bring if the driver changes what formats it reports. > > Is there anything that prevents packed formats from being preferred > > over planar/semi-planar ones? (I think the Intel decoder still supports > > packed, but I'm not sure.) > > No, and I don't think there needs to be. The user is asking for the decoder > to choose something from its own supported formats, and it does. If that > happens to be a format they can't cope with because they have additional > constraints then they needed to make it manually. Acknowledged. I guess there's only so much an automagic API can do for the user. (Unless we want to introduce something like get_hw_format, but that sounds silly and not much better than letting the user handle get_format.) On the other hand, shouldn't it attempt to pick the format that is likely to be most efficient? If there are even differences. It might also matter for reading back surface data, post processing via vavpp, and vaapi encoder input. What I'm trying to say is, maybe it would be good to make sure that a "standard" format like nv12 is picked instead of obscure "otherwise supported" formats. Unless that is already a given. > >> + > >> + // Start with at least four surfaces. > >> + ctx->surface_count = 4; > > > > (This still requires some additional work. The user should be able to > > control this. It's probably up to elenril to decide what exactly to do > > about this.) > > (Yes. Once the Completely General Official Method Of Setting Surface Pool > Sizes Everywhere exists then we will use it here too.) > > >> + // Add per-codec number of surfaces used for storing reference frames. > >> + switch (avctx->codec_id) { > >> + case AV_CODEC_ID_H264: > >> + case AV_CODEC_ID_HEVC: > >> + ctx->surface_count += 16; > >> + break; > >> + case AV_CODEC_ID_VP9: > >> + ctx->surface_count += 8; > >> + break; > >> + case AV_CODEC_ID_VP8: > >> + ctx->surface_count += 3; > >> + break; > >> + default: > >> + ctx->surface_count += 2; > >> } > >> + // Add an additional surface per thread is frame threading is enabled. > >> + if (avctx->active_thread_type & FF_THREAD_FRAME) > >> + ctx->surface_count += avctx->thread_count; > > > > I find it slightly odd that the surface_count field is always set, even > > of the user provides a hw_frames_ctx. But shouldn't be a problem. > > The make_config() function may also feed into an intended "test decoder > capabilities" API... Sure. Though I meant that the field is set even if the user sets a hw_frames_context, in which case the field will be unused and contain a "wrong" value that does not reflect reality. But ok, this is at most a potential nit. > > Last but not least, I'm wondering if it would be possible to not unref > > and recreate the hw frames pool if the surface format/size doesn't > > change. This would be useful for e.g. seeks. Seeks imply a > > avcodec_flush_buffers() call, which destroys the hwdec state and causes > > get_format() to be called again. > > The API requires us to call get_format(), and that implies that we have to > clear AVCodecContext.hw_frames_ctx (for current users, that API feature is > already fixed). > > So, this code could cache the frames context that it allocated last time and > then reuse it if the user offers the same device and the parameters are > identical? I think that would work, but it would also transiently allocate > more memory than it otherwise would when things change - is that ok? > Isn't the user disallowed to touch hw_frames_ctx if hw_device_ctx is set in the AVCodecContext? Which would mean the vaapi-specific init function could unref it before allocating a new frames context. On the other hand, it might be possible that most potential users of this feature will be fine with inefficient seeks. They can always go further by implementing it manually by setting get_format if more optimization is required. (With the main value of this new API being ease of entry, instead of having to implement semi-obscure callbacks.) _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
