Hi,
On Mon, Mar 12, 2018 at 05:04:44PM +0100, Lukáš Hrázký wrote:
> > I don't have numbers as I didn't consider it to be
> > significant at the time. My main concern was that it relies
> > on something not documented in the protocol itself (stream's
> > ID being a small number).
>
> But that is the old code relying on that, right? Any other
> reason than that to make this change? (I was thinking it was
> some preparation for adding the measurements)
Not really. Just that 'id' field matches better with a hash table
then a oversized array.
> I mean (theoretically! :)) if your patch was how the code was,
> somebody could write a patch to change it to what is the
> current (master) way with a message like:
>
> "Avoid the overhead of the hashtable by ensuring small stream
> IDs in the protocol and use an array to store the stream data."
>
> :D Not saying what you did is wrong, just that it depends on
> the context (which I know little of :)).
Right
> > > Might be a non-issue, just pointing it out as probably the only
> > > real consideration with this patch?
> >
> > Evaluating the difference between accessing an array an hash
> > table seems an overkill to me but, yeah, can be done...
>
> Not exactly what I meant. What I meant would be evaluating the
> difference between the whole codepath of processing one frame
> to the accessing of the hash table. We can safely say accessing
> an array is negligible :)
I'm confused. You just mentioned above that:
| "Avoid the overhead of the hashtable by ensuring small
| stream IDs in the protocol and use an array to store the
| stream data."
Could be a possible patch, etc. This patch changes from array to
hash table and that would be the only significant change on
"whole codepath of processing one frame" wouldn't it?
> So if the accessing of the hash table was say ~ 1% or 2% of the
> frame processing time, I would say in this case it's not worth
> the slowdown (of course YMMV with the number). The reason is we
> can quite safely ensure the array indexed by IDs is safe by
> asserting the size is lower than something reasonable and then
> (unless there is another reason) the hashtable has very little
> advantage.
>
> Of course I have no clue what is going on in the processing of
> one frame, it may well be that the number is like 0.001% and
> then it's a non-issue :)
So, this holds the display_stream structure which holds the
video_decoder field. By protocol, we can have multiple streams
and each stream could be in different formats like VP8, VP9,
H264, H265 and from a stream-id we need to get its display_stream
to add that payload to be decoded, etc.
e.g:
* st->video_decoder->queue_frame(st->video_decoder, frame, ...);
* st->video_decoder->reschedule(st->video_decoder);
> > I'm actually touching this part of the code thinking on
> > measurements but from the decoder's perspective (e.g from
> > SpiceFrame creation, decoding/rendering and deallocation).
>
> Not following here, I'm still not that familiar with the code.
> Needless to have a lenghty discussion with me while I keep
> guessing though :D I just wanted to point the one thing and I'm
> sure others can chime in with opinions on what's better and
> whether numbers are needed :)
Sure. What I mean is that, I'm trying to measure from the moment
we create and queue a SpiceFrame to the moment it is freed. We
can have several things going on on the Decoding side and it
would be cool to measure it, let me give you an example:
We have a mjpeg decoder but we could also decode with GStreamer.
GStreamer has more then one decoding element and even more
elements in its pipeline but, would be nice to do a fair
comparison and that's what I would like to achieve.
Another example is by having the almost exactly pipeline but
using Snir's patch [0] that renders on GPU. How much gain would
that be?
[0] https://lists.freedesktop.org/archives/spice-devel/2018-March/042688.html
This is OT for this patch of course, I'm just explaining because
I don't see a reason why not to. The reason I sent this patch is:
1) As stream-id field is not documented to be ascending from 0
(and in fact, it isn't with host doing the streaming) the current
code has flaws that could be addressed.
2) I end up sending patches for things that overall bothers me so
I can either fix something in the code or understand, from
other's views, where I'm mistaken.
Again, thanks for the review and discussion!
Cheers,
toso
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel