>
> > 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-requ...@ffmpeg.org with subject "unsubscribe".
