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

[Michael Niedermayer]

On Thu, Jun 13, 2024 at 10:50:36AM +0200, Andreas Rheinhardt wrote:
> 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?

i dont object, i was trying to explain why this was there. This predates
modern hw acceleration. The memory in AVFrame.data[] for a normal
pixel format can be mapped to GPU memory. FFmpeg itself probably did not
set it up this way but some applications did do this with get_buffer()
callbacks VERY long ago IIRC to avoid having to copy the image later
when the application belived the image would not be read from. (like a B frame
in MPEG-2 times)

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The day soldiers stop bringing you their problems is the day you have stopped 
leading them. They have either lost confidence that you can help or concluded 
you do not care. Either case is a failure of leadership. - Colin Powell
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240613/2c3e2911/attachment.sig>