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

"bit_stream buffer needs to be bigger" may this be an issue? #51

Open
eblanca opened this issue Apr 27, 2017 · 4 comments
Open

"bit_stream buffer needs to be bigger" may this be an issue? #51

eblanca opened this issue Apr 27, 2017 · 4 comments
Milestone

Comments

@eblanca
Copy link
Contributor

eblanca commented Apr 27, 2017

I found that for some particular encodings this message gets continously printed (no buffer overflows generated, now) and a corrupted bitstream is created.
Those are the cases of audio sources with very low samplerate encoded at very high bitrates (and free format encodings turn out to be the case): try encoding a 16 kHz source at 456 kbps and you'll have this.
Now, one may argue whether generating streams with such high bitrates may make sense, nevertheless lame allows for free format encodings up to 640 kbps and it would be great to have the same freedom in twolame.

Root: the root of this behaviour is into some library assumptions about bit_stream buffer (and its use), assumptions that may be wrong in those cases.

Fixes:

  1. if your primary goal is releasing ASAP a new library version, then a quick fix may be applied and it will be straightforward: set an upper bound for freeformat encodings (maybe 320 kbps / 400 kbps or such).

  2. if some time is available, then I would put some effort in fixing those assumptions and change how the library manages those buffers.

Tell me what you think, Nicholas.

@eblanca
Copy link
Contributor Author

eblanca commented Jul 3, 2017

The library manages two buffers: the incoming sample buffer and the outgoing bitstream buffer. The wrong assumption it does is: what is into the incoming buffer can be entirely encoded into the outgoing buffer, and we can see sometimes this can't be done. I think enlarging the outgoing buffer won't fix this, since its size comes from the application and nobody can tell how big the sample buffer will be. With a bigger sample buffer (more frames to encode) the issue will appear again.
Now, what if all the routines "twolame_encode_buffer*" change their behavior?
I mean, if they encoded only ONE single frame per call in version 0.4.0, this would be an API change and some already existing applications may stop working.
But things will definitely become easier, because the size of the needed bitstream buffer will be well known (and further, it can be smaller - it needs to hold a single frame). No matter how big the sample buffer will be (application taste), since the library will encode one single frame, and when encoding a single frame it needs at most (given source @ 16kHz and target mpeg stream @ 640 kbps - the most demanding one) 5760 bytes. No more, no less.

@njh njh added this to the v1.0 milestone Nov 27, 2017
@njh
Copy link
Owner

njh commented Nov 27, 2017

I don't have a very strong option on this one.

Pushing this issue out to version v1.0.

@eblanca
Copy link
Contributor Author

eblanca commented Jan 9, 2018

I'll soon create a PR to avoid the warning appearing any more.

@eblanca
Copy link
Contributor Author

eblanca commented Jan 9, 2018

Just created PR #78 for preventing this warning.
This is not a final solution, nevertheless the bitrate upper bound has to be checked anyways.

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

No branches or pull requests

2 participants