[vlc-devel] [PATCH 2/2] vlc_block: store per block ancillary

[Rémi Denis-Courmont]

Le lauantaina 7. heinäkuuta 2018, 17.32.57 EEST Denis Charmet a écrit :
> I see 2 cases where it will leak:
> 1. The block allocator overriding pf_release

Err? *All* block allocators provide their own pf_release. That's the whole 
point of pf_release being a function pointer.

> 2. The block_Chain functions that don't handle it

> But this could be easily fixed in the helpers.

Leaking - yes.

But reassigning side data to the right block in block chain - uh??

> Now out of curiosity I've tried to devise a way to do it out of band to
> compare
> 
> The only place where its storage could be easily carried out thoughout
> the different modules would be the es_format_t but:

We already extra p_extra/i_extra for non-timed side data. I assume this patch 
is for timed side data, so it won't fit in es_format_t.

AFAIK, there are basically two ways to pass them out of band:
- at ES output:
  - new control, or
  - new parameter to send callback control,
- at packetizer and decoder levels:
  - new optional callback, or
  - new parameter to decode/packetize callback.

New control and new callback would most certainly require much less code churn 
than new parameters.

> * The side data are bound to change over time they are not easily
> carried away out of band so what should hold them?
> 
> Let's suppose we make es_out_id_t the rightful owner of these data.
> Making sure that the handle is properly carried around with the
> es_format_t copies seems at best risky.
> 
> * Demux and packetizer should be able to feed them and associate it
> with blocks while encoders/sout_packetizer might be able to generate
> those kind of side data for the muxer.

Again, you can't have the meta-data magically go through packetizer, decoder, 
stream output. Most packetizers and decoders splice blocks between in and out 
- any new field of block_t will _not_ survive without special handling.

Also ES timeshift needs to serialize/deserialize. Again, the side data won't 
magically pass through just by adding a new block_t field.

> To be fair, I really think that whole concept of generic block has to
> die. Let's call it a frame like lavfc does and fix the cases where it's
> used as something else.

Notably access/stream uses block_t as a raw data buffer. That was previously 
data_buffer_t, while data_packet_t was for timed ES data (and sout_buffer_t, 
aout_buffer_t and pes_packet_t).

I agree that packet and buffer should be segregated back again. IMO, it should 
be done anyway, regardless of side data. But AFAIU, François is of the 
opposite opinion.

> We need to handle side data so short of embedding them inside the block
> data and hope for the best, I don't really see a simple way to do it
> without changing the block_t as we know it.

Ultimately, there are three ways to pass the data. New function calls, new 
call parameters or new structure fields.

If we had a specific enough structure, I would be fine with adding it there. 
But it won't be any easier than the other approaches. IMO, it just makes 
missing handling harder to spot, as the compiler won't warn.

-- 
レミ・デニ-クールモン
http://www.remlab.net/