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

Wrong index returned for AbrController.getQualityForBitrate #841

Closed
davidgarry opened this issue Oct 11, 2015 · 7 comments
Closed

Wrong index returned for AbrController.getQualityForBitrate #841

davidgarry opened this issue Oct 11, 2015 · 7 comments
Assignees
Milestone

Comments

@davidgarry
Copy link
Contributor

HI everybody,

I have the following bitrates list :

  • 0 : 400 000
  • 1 : 700 000
  • 2 : 1 000 000
  • 3 : 1 300 000
  • 4 : 1 800 000
  • 5 : 2 500 000

When I call the method MediaPlayer.setQualityFor('video', 5), the process call the method getQualityForBitrate(mediaInfo, 2500), and it returns 4 instead of 5, due to the line : return Math.max(i-1, 0);

In the block code below :

for (var i = 0; i < ln; i +=1) {
                bitrateInfo = bitrateList[i];

                if (bitrate*1000 <= bitrateInfo.bitrate) {
                    return Math.max(i-1, 0);
                }
}

So, I can't set the quality to 2500. Is there a reason to send the previous index when bitrates are equals ?

Regards,

@KozhinM
Copy link
Contributor

KozhinM commented Oct 12, 2015

@AkamaiDASH, looks like this code is a part of your implementation of fragment abandonment:
21a3026
I believe I remember hearing an explanation from you about this particular modification, but I do not remember the details and the reasons. I hope you do :)

@dsparacio
Copy link
Contributor

lol... it's foggy @KozhinM . I think it was to handle something on the opposite end when the bitrate sent in is lower the the lowest listed bitrate but I can not remember. I look for some email and test it. let me get to a place where i can test but looking at it and from the description @davidgarry it seems wrong and a bug. I'll follow up in a bit.

@davidgarry
Copy link
Contributor Author

@AkamaiDASH, do you have some news about this issue ?

@dsparacio
Copy link
Contributor

Seems like a valid bug but no have not looked yet and most likely will not for a bit. We are in the middle of a huge refactor so I am not in my normal state to look closely. Maybe just fix the one line you pointed out the notes in your code branch and we will fix in the next release. It would be very helpful if you could add a detailed issue to the issue tracker as well.

JibberJim added a commit to bbc/dash.js that referenced this issue Nov 4, 2015
JibberJim added a commit to bbc/dash.js that referenced this issue Nov 4, 2015
@JibberJim
Copy link

This is certainly a bug, simple fix committed on our branch as above, don't see the point creating a pull request while the refactor is ongoing, but let me know.

@dsparacio
Copy link
Contributor

This change did not make it up to 2.0 so I need to add bbc@6639326
to 2.1

@dsparacio
Copy link
Contributor

Fixed and directly pushed into dev without a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants