[FFmpeg-devel] [PATCH 33/57] avcodec/mpv_reconstruct_mb_template: Don't unnecessarily copy data

[Andreas Rheinhardt]

Michael Niedermayer:
> On Wed, Jun 12, 2024 at 03:48:29PM +0200, Andreas Rheinhardt wrote:
>> There is no reason to use a temporary buffer as destination
>> for the new macroblock before copying it into its proper place.
>>
>> (History: 3994623df2efd2749631c3492184dd8d4ffa9d1b changed
>> the precursor of ff_mpv_reconstruct_mb() to always decode
>> to the first row of macroblocks for B pictures when
>> a draw_horiz_band callback is set (they are exported to the caller
>> via said callback and each row overwrites the previously decoded
>> row; this was probably intended as a cache-optimization).
>> Later b68ab2609c67d07b6f12ed65125d76bf9a054479 changed this
>> to the current form in which a scratchpad buffer is used when
>> decoding B-pictures without draw_horiz_band callback, followed
>> by copying the block from the scratchpad buffer to the actual
>> destination. I do not know what the aim of this was. When thinking
>> of it as a cache optimization, it makes sense to not use it
>> when the aforementioned draw_horiz_band optimization is in effect,
>> because then the destination row can be presumed to be hot
>> already. But then it makes no sense to restrict this optimization
>> to B-frames.)
> 
> IIRC
> The B frames where directly placed in GPU memory, which is slow to read
> from, so building up MC + IDCT (which could read depending on caches)
> was avoided by a a scratchpad but its really long ago so i might remember
> this only partly
> 

The code here is not executed for hardware acceleration at all. For the
ordinary case, this copy incurs overhead; furthermore it increases
complexity (and for these reasons no other codec has similar code). So
I'd like to remove it (with an amended commit message that says that
this was done due to concerns about reading from GPU memory). Do you
object to this?

- Andreas