Can you edit the MR to remove the metadata commit?

Can you edit the MR to remove the metadata commit?

It would be a loss of time :-)

It would be a loss of time :-)

Then don't propose before the other MR has been merged, that's also a waste of time ;)

It would be a loss of time :-)

Then don't propose before the other MR has been merged, that's also a waste of time ;)

Well skipping the commit while reviewing isn't that hard ;)

Well skipping the commit while reviewing isn't that hard ;)

I'm not reviewing per-commit on github, comments there have a bad tendency to disappear on force push ..

Well skipping the commit while reviewing isn't that hard ;)

I'm not reviewing per-commit on github, comments there have a bad tendency to disappear on force push ..

Let's move it all to gst-plugins-rs ;)

In any case, this MR should be ready.

The EndSessionMessage is a bit unwieldy. I think I would rather do the following:

consumer (peer-id c) -> server -> producer (peer-id p)

StartSession peer_id=p  -> StartedSession session_id=s peer_id=c ->
                        <- StartedSession session_id=s peer_id=p
EndSession session_id=s -> EndedSession session_id=s peer_id=c ->

What do you think?

Sounds like you need to maintain more state in both peers to retain the session ID which you do not care about in most cases so I don't see how that simplifies anything in practice.

Sounds like you need to maintain more state in both peers to retain the session ID which you do not care about in most cases so I don't see how that simplifies anything in practice.

It simplifies things by removing an error case (the peer exists but is not registered as the provided type). Besides that, the message relates to ending a session, so expecting whoever wants to do that to keep a trace of a session ID isn't that unreasonable :)

It simplifies things by removing an error case (the peer exists but is not registered as the provided type).

Well, it means you have a different set of possible errors yes, not sure one error set is simpler or not (in the code detecting this specific error is very simple).

so expecting whoever wants to do that to keep a trace of a session ID isn't that unreasonable

Unreasonable no, not necessary, yes.

Unreasonable no, not necessary, yes.

Well I think it is necessary to get this merged :)

ping @meh

Thanks for taking the time to refactor in-depth, this new protocol is a clear improvement :) Do you think all four commits are useful to have in the history, or should we fixup some or all of them?

I'm OK with the changes btw

Thanks for taking the time to refactor in-depth, this new protocol is a clear improvement :) Do you think all four commits are useful to have in the history, or should we fixup some or all of them?

I think they make sense, I ended up reworking the history in a way that made sense to me. If you want it some other way, please let me know.

I didn't look at how the individual commits were split up tbh, if you say the split up makes sense let's go for it :)

Thanks :)