[vlc-devel] [PATCH] input/stream: do not unconditionally invalidate block on seek
Filip Roséen
filip at atch.se
Mon Oct 24 16:27:05 CEST 2016
Hi Rémi,
On 2016-10-24 16:02, remi at remlab.net wrote:
> Hello,
>
> I don't follow. The "penalty" of block-based access to stream-based
> demux is a memory copy. It is intrinsic.
>
I am not talking about the *memory copy* taking place in order to copy
data from the generated block into the destination buffer referred to
by an invocation of `vlc_stream_Read` (and related functions), that is
(as you said) something intrinsic to the mechanisms in play.
I am talking about the *memory copy/write* that inherently must take
place in order for a block-based content *"generator"* to create a
`block_t` that is then living inside `src/input/stream.c`.
[ block access ] -> [ block cache ] -> [core] -> [ stream reader ]
^- A ^- B ^- C
/**
* A + B is somewhat redundant to the example, we could have left
* out B and have the same behavior, but it is added to better
* reflect how the previous discussion.
*
* One should however keep in mind that there might be any number
* of stream_filters between "[core]" and "[stream reader]", and
* if these are block-based things get worse.
**/
In the above `B` is simply a cache that will cache all the blocks
that are generated by `A`, the `[core]` is the implementation within
`src/input/stream.c`, and `C` is something reading using
`vlc_stream_Read`.
Imagine we are att offset zero, and `B` generates blocks `1 MB` in
size, a block having `p_block->i_buffer = 1024*1024` would end up in
`[core`].
If `C` wants to read `256` bytes at offset zero, and then `256 bytes`
at offset `524 288` it would (probably) do something as the below
(even though it could use skipping, but it has no way of knowing that
`B` has `N` as "chunk size"):
vlc_stream_Read( src, buf_1, 256 ); /* (1) */
vlc_stream_Seek( src, 524288 ); /* (2) */
vlc_stream_Read( src, buf_2, 256 ); /* (3) */
Given that we currently, unconditionally, invalidate read blocks in
`[core]` during `vlc_stream_Seek`, at `(3)` we would have to ask `B`
to create a new block which the `[core]` transparently will use to
feed `C`.
In other words, `B` will "regenerate" contents that was already
available at `(1)`, and for *accessors/stream_filters* that can only
fetch chunks of `N` bytes of data upstream, it would have to fetch `N`
bytes, even though reading stream is only interested in a smaller
subset.
Now imagine that `C` is seeking back and forth within a single block;
- why not have the block live inside `[core]` until it is definitely
exhausted, instead of asking `B` (or `A`) to generate new contents
only to discard it?
> Furthermore, the micro-optimization in this patch does not remove or
> add any memory copy as such.
The *"micro-optimization"* gets rid of asking the source,
*block-based*, stream to give data that was already available prior to
`vlc_stream_Seek`.
I could provide a *testcase* and benchmark if this is
wanted/requested.
> Le 24 oct. 2016 2:21 PM, Filip Roséen <filip at atch.se> a écrit :
> Hi Rémi,
>
> I honestly feel like you are misunderstanding what I am trying to
> convay, if that is me being vague in my explanation or if we are
> just talking different languages - I do not know.
>
> Given that we have a transparent proxy going from block-based
> reading to what most people associate with “stream”, it is very
> unfortunate that block-based accessors suffer a penalty because of
> that.
>
> The reading on the “output” end should not have to worry about the
> resource which it is reading data from, and should certainly /not/
> care whether the input-side is a _block-_ or _stream-based_ stream.
>
> In my opinion it does not make any sense for us to copy the same
> data over and over, no matter if one issues a seek or does a proper
> read (skip) into a NULL-buffer.
>
> Le maanantaina 24. lokakuuta 2016, 10.38.03 EEST Filip Roséen a écrit :
>
> Hi Remi
>
> On 2016-10-24 11:33, Rémi Denis-Courmont wrote:
>
> Le maanantaina 24. lokakuuta 2016, 2.13.11 EEST Filip Roséen a écrit :
>
> If a seek request happens to refer to a position within the already
> read block, simply update the block so that it refers to the desired
> data.
>
> There is really no need for us to discard the block, only to ask the
> underlying stream to give us a new one (when we have the data we want
> already).
>
> I don´t see the point. I have spent a lot of time untangling the caching
> from the generic byte stream reader, and this is just starting to bring
> it back in and duplicate functionality.
>
> I agree that we should keep the implementation simple and consise, and
> not duplicate functionality; but having the kind of support proposed
> is just a extremely minor subset of the functionality within other
> parts of the code..
>
> Any reasonable cache implementation will optimize a short forward skip or
> read. That is the whole point of read-ahead caching. So this is completely
> pointless complexity.
>
> This is besides the point.
>
> and I think it make sense not to re-read data
> (just as we have the functionality in place for peeking).
>
> Yes. That´s what VLC and operating systems have caches for.
>
> VLC has a conditional cache, and when these cache modules not apply,
> performance suck; that’s it.
>
> And on that note, the current cache mechanisms are applied too early
> in the chain - given that _stream_filter_ (applied after the
> _accessor_ is created) cannot take advantage of the cache-modules,
> reading from a block-based _stream_filter_ suffers from the “there
> is no cache”-flaw, no matter what.
>
> One could even merge the peeking and block "cache" in
> `src/input/stream.c`, making the implementation even less complicated
> than it is now.
>
> Support for peek and block is already in-there. Nothing else needs to be
> merged, and anything more will obviously make the code more and needlessly
> complicated.
>
> Support for peeking exists, the support of block in this regard is
> certainly not as good as it could be (and given that you just said
> that such complexity is not supposed to be there, you inherently say
> that support for not copying the same data over and over is not
> there).
>
> If you need to skip forward, call vlc_stream_Read(..., NULL, ...) in the
> demuxer or wherever else needed. That will work even if the stream is not
> readable.
>
> I guess we should have a major clean-up session, because we have
> demuxers (such as `modules/demux/mp4` and `modules/demux/avi`), and
> other entities, that does a lot of seeking back and forth, and for
> block-based accessors this is killing performance (really bad).
>
> This is FUD. Bringing cache_block back instead of prefetch will not improve
> uninterleaved file playback. cache_block has no support for multiple tracks,
> and never had. It is much worse than prefetch for that purpose.
>
> Besides, reading uninterleaved AVI through a network has always sucked in VLC.
> Even if you remove the prefetch and new HTTP plugins, cache_stream's support
> for multiple tracks will only fix it partially (and it will break what
> prefetch fixed such as high-bandwidth non-seeking streams). We know that from
> old user feedback.
>
> There is only one definite way to fix playback of uninterleaved tracks: set up
> one byte stream per track (at least per active track). Using multiple tracks
> with a single stream is mathematically bound to eventually fail:
>
> Take:
> - T the number of active tracks or read offsets,
> - N the available network bandwidth,
> - S the stream bitrate, and
> - L the seek latency.
>
> You need to accumulate buffers of T * S * L and then some, only to compensate
> for seeking. You can build up that buffer at best at a rate of: N - S.
>
> So you need to buffer for a duration of at least: T * S * L / (N - S)
> And allocate corresponding buffer space of: T * S² * L / (N - S)
>
> Note that this goes out-of-hand if N nears S, or if S is large. And that
> assumes you can even estimate those values.
>
> As for a pure random access formatted file, you have to play from a random
> access media. It is unfixable at software level.
>
>
> I am well aware of the above, but I honestly feel like bringing up
> such explanation of how things work is drawing attention away from
> the real issues:
>
> - We are copying data over and over if you happen to seek within
> what is the same block for a _block-based_ stream.
>
> - Given that the _reader-side_ cannot know the size of the block
> coming from the _input-side_ (unless it is reading blocks):
>
> - How can it know that a _read-skip_ is better than seek (given
> that the block size can be arbitrary)?
>
> - Should we force implementations not to issue seek-requests at
> all, and just rely on skipping?
>
> - Why not make the skipping and seeking be based on the same
> implementation? It is semantically the same thing.
>
> This is not a matter of adding complexity, and as I already said
> rewriting the relevant part of src/input/stream.c so that the
> functionality is shared would even _simplify_ the implementation
> (not the opposite).
>
> I do not know what else to add, I am honestly quite baffled about
> the negativity received;
>
> - what do you propose to fix the problem, or;
> - are you saying that there is no problem to be fixed?
>
> Best Regards,
> Filip
>
> Sure, they can be fixed to not discard already read data, but I find
> that having simple management of blocks within `src/input/stream.c` is
> not too complex.
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161024/6b03c450/attachment.html>
More information about the vlc-devel
mailing list