Add interactive pause/resume functionality #14
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We need to reset the file cursor to the beginning of the file. Initially, the program would simply get stuck polling the compress fd. This was probably because of the fact that the codec would hang because of the fact that it was expecting to receive the MP3 data along with its associated MP3 header. This was not the case for the first (header, data) pair because, after parsing the first header, the file cursor would point at the beginning of the data region. By resetting the file cursor to the beginning of the file, the codec will receive all the (header, data) pairs it actually expects. Suggested-by: Shengjiu Wang <shengjiu.wang@nxp.com> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
dbaluta
reviewed
Aug 29, 2022
LaurentiuM1234
force-pushed
the
add_pause_resume
branch
from
0086823
to
57096e5
Compare
dbaluta
reviewed
Aug 29, 2022
LaurentiuM1234
force-pushed
the
add_pause_resume
branch
from
57096e5
to
9df3708
Compare
|
Update
|
dbaluta
approved these changes
Sep 5, 2022
|
@perexg @vinodkoul can you please help review this? |
dbaluta
reviewed
Sep 6, 2022
LaurentiuM1234
force-pushed
the
add_pause_resume
branch
from
9df3708
to
16cb691
Compare
|
Update:
|
LaurentiuM1234
force-pushed
the
add_pause_resume
branch
from
16cb691
to
153e1a0
Compare
|
@perexg Should be fine now |
dbaluta
approved these changes
Sep 7, 2022
perexg
approved these changes
Sep 7, 2022
vinodkoul
reviewed
Sep 8, 2022
vinodkoul
reviewed
Sep 8, 2022
vinodkoul
requested changes
Sep 8, 2022
LaurentiuM1234
force-pushed
the
add_pause_resume
branch
from
153e1a0
to
96c46c8
Compare
plbossart
reviewed
Sep 12, 2022
There was a problem hiding this comment.
I would suggesting avoiding the term 'resume', it leads to confusions between pause_push/pause_release and suspend/resume.
Also I am not sure what happens with compressed formats such as AAC_ADIF, where the information configuration is provided initially and not with a header. There might be a need to prevent the DSP from suspending to avoid losing context?
| is_paused = true; | ||
| break; | ||
| case DO_RESUME: | ||
| if (compress_resume(compress) != 0) { |
There was a problem hiding this comment.
I think this should be addressed in another series of patches as this one doesn't actually introduce compress_resume() but it just uses it.
There was a problem hiding this comment.
Agreed, this should be done as a different change
LaurentiuM1234
force-pushed
the
add_pause_resume
branch
from
96c46c8
to
175b8ff
Compare
We want to be able to pause and resume the stream just like the ALSA utilitary aplay does. In order to do so, we first need to mark the read operations on stdin as nonblocking and then enable the noncanonical mode for the terminal. What this does is it makes the read operations nonblocking and it makes the input available immediately. After doing so, we can check if we receive a SPACE or ENTER character from user and do pause/resume depending on current stream state (and by this I mean if the stream is currently paused and we receive a SPACE or ENTER then we resume it and vice-versa) Signed-off-by: Maurine Creton <maurine.creton@nxp.com> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
|
@vinodkoul @plbossart @perexg I think we are good with this. All the comments have been addressed. |
vinodkoul
approved these changes
Sep 19, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We want to be able to interactively pause and resume the stream.
Initially, when trying to play an MP3 song, cplay would hang during the polling on compress_write. The cause for this was the fact that, after calling
get_codec_mp3, the file pointer would point to the first MP3 data, leaving out the first MP3 header. I'm assuming that this would cause the codec to be unable to process the received frames since it wouldn't be able to find the first MP3 header.In order to be able to use the pause/resume functionality on MP3 streams, this problem had to be addressed before adding the actual functionality.