[vlc-devel] RIST Protocol: remove old rist modules and add new ones based on the librist library

[Ilkka Ollakka]

On Sat, Apr 18, 2020 at 01:26:42PM -0400, Sergio M. Ammirata, Ph.D. wrote:
> Hi,

Hi,

> Who owns the memory space was a topic of much internal
> discussion.

I'm sure, and there are usually huge amount of opinions and pros/cons to
choose from. But something that you might want to think is should
librist offer functions to define callbacks to do custom
buffer-management for applications 'give me buffer, release this buffer'
so applications can do their own implementations and in this case vlc
modules could implement vlc_block based buffers and get away most of the
memcpy in modules.

It's not trivial thing to get running nicely, but just a point of view
that most likely amount of small sized memcpy all over the codeflow
causes performance drop that could be avoided.

> On the sender side (access_output module):

> We have to keep the data around for a long time (every
> packet for 1 second or more depending on settings) to
> fulfill re-trasmissions requests. This requirement forces
> to use our own fifo and copy the data out of the calling
> app into our fifo as soon as we get it. 

> That being said, we could take the approach you suggest and
> loop internally within the library to "split" it into the
> proper packet sizes there and save one or more memcpy
> operations outside. I will make that change and re-submit.


> On the receiver side (access module):

> We have two ways to pass data to the calling app and we
> left both in the code for comments. One uses a callback and
> one uses a poll with a timeout against a fifo of length of
> 1000. In either case, we pass a pointer to the fifo
> structure (a rist_data_block) to the app and save a memcpy
> internally as these fifos our only freed during library
> shutdown (data is valid for 1000 packets). Technically, the
> app could use the data pointer directly without a memcpy
> with the caveat that if it takes too long to process it
> (1000 packets later), the data they read would belong to a
> much newer packet and other unpredictable results would
> follow.

The problem with receiving side is also that it forces each BlockRIST
call to do small memcpy/allocation instead of just defining that
receiver needs to free the buffers when they are no longer used by the
caller? In this case you could just use block_Init with buffer-pointer
and size to get the block without memcpy at each point.

Does librist needs those received buffers for something (my knowledge on
rist are not that deep, so if it's something obvious or core of rist,
you can just point to documentation describing thing)?


> Regarding the callback vs poll, I think poll matches the
> architecture of the "BlockRist" loop better than having the
> additional overhead of a fifo (unless I missed something).
> Feedback/thoughts are welcome.

Code wise they seem to do same alloc/memcpy, I don't have opinion on
myself on that.

> Regards,
> Sergio


> -----Original Message-----
> From: Ilkka Ollakka <ileoo at videolan.org>
> Reply-To: Mailing list for VLC media player developers <
> vlc-devel at videolan.org>
> To: Mailing list for VLC media player developers <
> vlc-devel at videolan.org>
> Subject: Re: [vlc-devel] RIST Protocol: remove old rist
> modules and add new ones based on the librist library
> Date: Sat, 18 Apr 2020 17:04:31 +0300

> On Fri, Apr 17, 2020 at 08:09:35PM -0400, Sergio M.
> Ammirata, Ph.D. wrote:
> I have created a new fork with just two commits to make
> iteasy to read. One commit deletes the old modules and
> thenext one creates the new ones.Can I get some feedback
> please? I am ready to submit amerge request.
> https://code.videolan.org/rist/vlc/-/commits/rist-devThanks
> ,
> Hi,
> Didn't yet read the whole module code through, but I
> spotted thatmodules use memcpy on quite core of write/read
> functions, in most casesit means that they do lot of small
> copies. And in littlebit later youcheck the side you have
> on creating rist_buffer and writing.
> On BlockRist this looks like only way to do it for now, as
> libristdoesn't seem to support providing own allocation
> function. So similarapproach that is used in in avcodec-
> modules doesn't seem to be possible.
> In access_output/rist.c:Write, Why not just queue blocks as
> they are andwrite block at a time (and reduce it if it
> doesn't fit), as you anywayloop until the buffer is empty?
> That looks like possible approach,unless I missed something
> on how librist handles given buffers?



> _______________________________________________vlc-devel
> mailing listTo unsubscribe or modify your subscription
> options:https://mailman.videolan.org/listinfo/vlc-devel

-- 
Ilkka Ollakka
He that teaches himself has a fool for a master.
		-- Benjamin Franklin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200419/6e3800c3/attachment.sig>