Re: [FFmpeg-devel] [PATCH, v6] avcodec/d3d12va_encode: texture array support for HEVC

[Araz <primeadvice@gmail.com>]

>
> > av_calloc(MAX_DPB_SIZE,sizeof(D3D12_RESOURCE_BARRIER));
>
Need an extra space `MAX_DPB_SIZE,sizeof` -> `MAX_DPB_SIZE, sizeof`
>
Good point, I will fix this in the upcoming patch version.


> > + uint32_t mip_slice, plane_slice, array_slice, array_size;
>
use the following codes and you can remove this line:
>     uint32_t arra_size   = ...
>     uint32_t mip_slice   = ...
>     uint32_t array_slice = ...
>     for (uint32_t plane_slice = 0; ... )
>
Acknowledged, will be fixed in the upcoming patch version.

>
> > +            for (plane_slice = 0; plane_slice < ctx->plane_count; plane_slice++) {> +                //Calculate the subresource index> +                uint32_t planeOutputSubresource = mip_slice + array_slice *> references_tex_array_desc.MipLevels +> +                                        plane_slice * references_tex_array_desc.MipLevels *
>
> > array_size;
>
planeOutputSubresource is wrong naming and better add this statement
>
as a function: d3d12va_cal_subresource_index.
>
I understand the suggestion, but in this case introducing a separate
function like d3d12va_cal_subresource_index() feels like
unnecessary overhead.
The calculation is straightforward and done in a local context with
readily available variables, so passing multiple parameters to a helper
function would make the code more verbose without adding much clarity.
That said, I agree that the variable name planeOutputSubresource
is not ideal — I will rename it in the upcoming patch version.

>
> > + //swap the barriers_ref transition state
>
No need. The code has been telling us that it is swapping the barrier state.
>
Will be fixed in the upcoming patch version.


> > +    if (barriers_ref_index > 0) {> +        for (i = 0; i < barriers_ref_index; i++) {> +            D3D12_RESOURCE_STATES temp_statue => barriers_ref[i].Transition.StateBefore;> +            barriers_ref[i].Transition.StateBefore => barriers_ref[i].Transition.StateAfter;> +            barriers_ref[i].Transition.StateAfter = temp_statue;
>
>  > + }
>
Can we use FFSWAP?
>
Will be fixed in the upcoming patch version.

>
>
> + hwctx->texture_array_size = ctx->is_texture_array ? MAX_DPB_SIZE +
>
> + 1 : 0;
>
Can we use initial_pool_size?
>
Thank you for the suggestion.
But Initial_pool_size has different purpose.
It defines the number of surfaces in the hardware frame pool,
but in this case, texture_array_size controls
the number of subresources within a single texture array.

> > +    /**> +     * Flag indicates if the HW is texture array mode.> +     */> +    int is_texture_array;> +
>
> Do we really need it? If the texture_array of hwframectx is not NULL, it
> is texture array mode, right?
>
It is set early in the encoder initialization phase,
before the texture_array_size is configured and
the texture_array resource is created.
Since hwctx->texture_array is only initialized later
in the pipeline, it can't be reliably used as an indicator
for texture array mode during earlier stages.
Also, is_texture_array is checked multiple times throughout the code,
and having a dedicated flag improves code readability.
Keeping this flag is justified for both correctness and clarity.

> > +    if (hwctx->texture_array) {> +        D3D12_OBJECT_RELEASE(hwctx->texture_array);> +        hwctx->texture_array = NULL;> +    }>  }
>
> D3D12_OBJECT_RELEASE(hwctx->texture_array) is enough.
>
D3D12_OBJECT_RELEASE will release it and set it to NULL if it's not NULL.
>
 Will be removed in the upcoming patch version.

> > +    //For texture array mode, create texture array resource in the init stage.> +    //This texture array will be used for all the pictures,but with different> subresources.
>
> I strongly recommend removing all the comments in the patch. They are just
> basic
> knowledge of the Graphics API and an usage of the texture array.
>
 Will be removed in the upcoming patch version.

> > +            av_log(ctx, AV_LOG_ERROR, "Could not create the texture\n");
>
> Perhaps `Could not create the texture array`?
>
 Will be fixed in the upcoming patch version.

>
> Could you add it to a separate function like
> d3d12va_texture_array_mode_init?
>
 Will be fixed in the upcoming patch version.

> > +    /**> +     * In texture array mode, the D3D12 uses the same the same texture array> (resource)for all> +     * pictures.
>
> *the same the same*
> Is it a typo?
>
 Thanks, will be fixed in the upcoming patch version.

> > +    int texture_array_size;
>
> There is an initial_pool_size in AVHWFramesContext. As the comment says,
>     * Initial size of the frame pool. If a device type does not support
>     * dynamically resizing the pool, then this is also the maximum pool
> size.
> Can we reuse it instead of adding this member?
>
While initial_pool_size controls how many frames are in the pool,
texture_array_size governs how many subresources exist inside
the shared texture array - it's a different layer of resource allocation.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".