Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overview of a=imageattr handling #150

Merged
merged 3 commits into from
Jun 16, 2015
Merged

Overview of a=imageattr handling #150

merged 3 commits into from
Jun 16, 2015

Conversation

juberti
Copy link
Contributor

@juberti juberti commented Jun 12, 2015

Still needs updates to createOffer/Answer. Fixes #5

@ekr
Copy link
Contributor

ekr commented Jun 12, 2015

Travis still seems sad.

receiver wants to receive, it will intersect its decoder hard
limits with any mandatory constraints that have been applied to
the associated MediaStreamTrack. The resulting resolution space
will be used as the x= and y= values for the generated "a=imageattr"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unclear. Is your idea that this was you do applyConstraints in ontrackadded. Perhaps some more text would help.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, or really any time prior to SetLD(answer).

On the offerer side, this is problematic, as the track doesn't exist prior to setRD. While this could be addressed by a separate offer-answer exchange, this suggests that this mechanism is not ideal.

Agree the text should discuss this.

@ekr ekr added the r+ekr label Jun 12, 2015
@ekr
Copy link
Contributor

ekr commented Jun 12, 2015

Since I won't be on the call, I give this my preemptive LGTM, provided you at least look at my comments (and maybe reject them) and fix the build

@juberti
Copy link
Contributor Author

juberti commented Jun 14, 2015

Eric, did you want to take another look at this?

@ekr ekr added the r?fluffy label Jun 14, 2015
does not know about any remote MediaStreamTracks until it receives
theanswer, this implies that it can only set these constraints after
the initial offer-answer exchange is complete, and as a result, a
new offer-answer is needed to communicate them.</t>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A sends offer to B. Lets say it has

a=imageattr:97 send * recv *

B wants to limit it was it receives to 360p and sends back an pr-answer with

a=imageattr:* recv [x=[16:640],y=[16:360],q=1.0]

and A starts sending 360p to B when it gets this pr-answer.

I'm not seeing why an 2nd offer/answer is needed. You do an answer of pr-answer before the constraint can take place but given an pr-answer will work, I don't think it is right to say that the offer-answer needs to complete.

It seems like replacing "the initial offer-answer exchange is complete, and as a result, a new offer-answer is needed to communicate them" with "receiving the initial answer or pr-answer. " would fix this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you may be interpreting the text here different from what is intended.

Say that we take the example you give except that once A calls setRD(answer) on B's answer, the JS applies a constraint to the resulting media stream that restricts it to 360p. There is no way for A to communicate that to B in this exchange, because it has concluded, and B is left thinking that it can send at any rate. In order to adjust B's ideas, A needs to send a new offer with a new a=imageattr. Do you disagree with this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - I think I'm starting to get one this is getting at. Let's separate two cases here. Case 1 is A is mobile phone that the actually browser knows that due to hardware limitations it can not receive more than 360p, and case 2 where it's a desktop browser that can receive big stuff but the JS app wants to limit this particular video stream to 360p.

In case 1, the limit that A can only receive 360p would have been in the initial Offer.

In case 2, what you are saying is that as the JS API layer, we have no way to set this limit until after an answer or pr-answer is received by A? It seems we need to fix this API layer not so much for this but more for simulcast.

Agree of course that any time A wants to change it's constraints after sending the Offer, it's going to need to do new O/A exchange.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your analysis here, but it seems like for now we should be landing effectively what Justin proposed. Is there something you think would make this clearer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I thin this case analysis is correct, and yes, I think this does reveal a problem in the
API, which you would presumably need to resolve by having some sort of template in
offerToReceiveFoo.

@fluffy
Copy link
Contributor

fluffy commented Jun 15, 2015

There one sentence I think we need to deal with (see other comment) but rest of this LGTM

@fluffy
Copy link
Contributor

fluffy commented Jun 15, 2015

It still seems like my proposed change of replacing "the initial offer-answer exchange is complete, and as a result, a new offer-answer is needed to communicate them" with "receiving the initial answer or pr-answer. " would make this more or less right for both cases and as the API developers, or does not develop, for this then we can fill in more details

@ekr
Copy link
Contributor

ekr commented Jun 15, 2015

Well, as far as I can tell, you can't do it after the pr-answer, since you can't do an offer at that stage.

@ekr
Copy link
Contributor

ekr commented Jun 15, 2015

@juberti PTAL at PR #153. I think with this change we are ready to go. If that makes you happy, merge in PR #153 into here and then merge this PR.

@fluffy
Copy link
Contributor

fluffy commented Jun 15, 2015

With EKR's #153 this LGTM

@juberti
Copy link
Contributor Author

juberti commented Jun 16, 2015

OK, I will push the buttons now.

@juberti
Copy link
Contributor Author

juberti commented Jun 16, 2015

OK, I worked this over a bit to incorporate PR #153 and also handle the case where the decoder hard limits are not known. The changes are minor but I will hold off for the next few hours to get a sanity check from one of you.

@ekr
Copy link
Contributor

ekr commented Jun 16, 2015

LGTM

@fluffy
Copy link
Contributor

fluffy commented Jun 16, 2015

I will read it and get back to you I 30 minutes

@fluffy
Copy link
Contributor

fluffy commented Jun 16, 2015

LGTM

@fluffy fluffy added r+fluffy and removed r?fluffy labels Jun 16, 2015
@ekr ekr merged commit 9aac608 into master Jun 16, 2015
@ekr
Copy link
Contributor

ekr commented Jun 16, 2015

Fixed merge conflict and merged.

@juberti juberti deleted the imageattr2 branch August 14, 2015 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants