Re: [alsa-devel] [RFC] compress: add support for gapless playback
On Thu, Feb 07, 2013 at 01:15:54AM +0000, Richard Fitzgerald wrote:
+- partial drain +This is called when end of file is reached. The userspace can inform DSP that +EOF is reached and now DSP can start skipping padding delay. Also next write +data would belong to next track
We're really doing two different tasks inside the "partial drain". The name, and the reason we are doing two tasks in one, comes from a particular higher- layer scenario, but there's no reason the driver API need use the same terminology as one particular application of the functionality. The two tasks are:
- Tell the DSP that we have sent all data for the current track and following
data will be for the next track. This lets the DSP lay down a marker for where it should strip padding at the end of a track, and know it should be expecting more data for another track to follow gaplessly where it must strip the encoder delay from the start.
- Ask for notification when DSP has reached the changeover point between the
playback of the two tracks
I think it would be more logical and less confusing not to combine the two into a single ioctl. Instead, add only one new ioctl specifically to provide the track changeover hint, something like SNDRV_COMPRESS_NEXT_TRACK (meaning DSP should expect data for next track to follow). Don't add a new drain, just use the existing SNDRV_COMPRESS_DRAIN - the driver/DSP can make a decision whether it needs to do something special with the drain if we have told it that we will be sending it some more data for a following track.
So the SNDRV_COMPRESS_PARTIAL_DRAIN in this patch would become
- Send SNDRV_COMPRESS_NEXT_TRACK
- Send SNDRV_COMPRESS_DRAIN
The problem would be in that case the defination of SNDRV_COMPRESS_DRAIN which expects the decoder to completely drain its buffers and come to complete halt. This would also mean the framework will treat a drained stream as stopped and needs a new start. Certainly we dont want that in this case. So we can't use SNDRV_COMPRESS_DRAIN to indicate. Yes we can put conditional check but IMO that would overtly complicate this. If we are not doing proper drian lets not call it that.
But I think I like the idea of splitting the two up to do a cleaner interface. Let me check this...
When we reach the final track we just do SNDRV_COMPRESS_DRAIN to wait for it to finish.
If we setup a next track by doing SNDRV_COMPRESS_NEXT_TRACK and then change our mind and decide that this is going to be the last track, we do a SNDRV_COMPRESS_DRAIN without sending any next track data, then we do SNDRV_COMPRESS_STOP
Nope that is where we would have issue. You dont call SNDRV_COMPRESS_STOP for compressed streams. You have to wait till DSP has decoded and rendered data for the last track which can be done by using SNDRV_COMPRESS_DRAIN only. SNDRV_COMPRESS_STOP should not be called in this case.
-- ~Vinod
At Wed, 6 Feb 2013 18:18:07 -0800, Vinod Koul wrote:
On Thu, Feb 07, 2013 at 01:15:54AM +0000, Richard Fitzgerald wrote:
+- partial drain +This is called when end of file is reached. The userspace can inform DSP that +EOF is reached and now DSP can start skipping padding delay. Also next write +data would belong to next track
We're really doing two different tasks inside the "partial drain". The name, and the reason we are doing two tasks in one, comes from a particular higher- layer scenario, but there's no reason the driver API need use the same terminology as one particular application of the functionality. The two tasks are:
- Tell the DSP that we have sent all data for the current track and following
data will be for the next track. This lets the DSP lay down a marker for where it should strip padding at the end of a track, and know it should be expecting more data for another track to follow gaplessly where it must strip the encoder delay from the start.
- Ask for notification when DSP has reached the changeover point between the
playback of the two tracks
I think it would be more logical and less confusing not to combine the two into a single ioctl. Instead, add only one new ioctl specifically to provide the track changeover hint, something like SNDRV_COMPRESS_NEXT_TRACK (meaning DSP should expect data for next track to follow). Don't add a new drain, just use the existing SNDRV_COMPRESS_DRAIN - the driver/DSP can make a decision whether it needs to do something special with the drain if we have told it that we will be sending it some more data for a following track.
So the SNDRV_COMPRESS_PARTIAL_DRAIN in this patch would become
- Send SNDRV_COMPRESS_NEXT_TRACK
- Send SNDRV_COMPRESS_DRAIN
The problem would be in that case the defination of SNDRV_COMPRESS_DRAIN which expects the decoder to completely drain its buffers and come to complete halt. This would also mean the framework will treat a drained stream as stopped and needs a new start. Certainly we dont want that in this case. So we can't use SNDRV_COMPRESS_DRAIN to indicate. Yes we can put conditional check but IMO that would overtly complicate this. If we are not doing proper drian lets not call it that.
But I think I like the idea of splitting the two up to do a cleaner interface. Let me check this...
In the way above, NEXT_TRACK is no longer a trigger op, but it's rather an operational option. This would set a flag something like compr->drain_mode = SND_COMPR_DRAIN_RESTART; while its default is SND_COMPR_DRAIN_STOP. (Or, set compr->next_track_after_drain = true, or whatever.)
Though, I'm also not sure whether the scenario above improves the situation better...
Takashi
On Thu, Feb 07, 2013 at 09:49:37AM +0100, Takashi Iwai wrote:
At Wed, 6 Feb 2013 18:18:07 -0800, Vinod Koul wrote:
On Thu, Feb 07, 2013 at 01:15:54AM +0000, Richard Fitzgerald wrote:
+- partial drain +This is called when end of file is reached. The userspace can inform DSP that +EOF is reached and now DSP can start skipping padding delay. Also next write +data would belong to next track
We're really doing two different tasks inside the "partial drain". The name, and the reason we are doing two tasks in one, comes from a particular higher- layer scenario, but there's no reason the driver API need use the same terminology as one particular application of the functionality. The two tasks are:
- Tell the DSP that we have sent all data for the current track and following
data will be for the next track. This lets the DSP lay down a marker for where it should strip padding at the end of a track, and know it should be expecting more data for another track to follow gaplessly where it must strip the encoder delay from the start.
- Ask for notification when DSP has reached the changeover point between the
playback of the two tracks
I think it would be more logical and less confusing not to combine the two into a single ioctl. Instead, add only one new ioctl specifically to provide the track changeover hint, something like SNDRV_COMPRESS_NEXT_TRACK (meaning DSP should expect data for next track to follow). Don't add a new drain, just use the existing SNDRV_COMPRESS_DRAIN - the driver/DSP can make a decision whether it needs to do something special with the drain if we have told it that we will be sending it some more data for a following track.
So the SNDRV_COMPRESS_PARTIAL_DRAIN in this patch would become
- Send SNDRV_COMPRESS_NEXT_TRACK
- Send SNDRV_COMPRESS_DRAIN
The problem would be in that case the defination of SNDRV_COMPRESS_DRAIN which expects the decoder to completely drain its buffers and come to complete halt. This would also mean the framework will treat a drained stream as stopped and needs a new start. Certainly we dont want that in this case. So we can't use SNDRV_COMPRESS_DRAIN to indicate. Yes we can put conditional check but IMO that would overtly complicate this. If we are not doing proper drian lets not call it that.
But I think I like the idea of splitting the two up to do a cleaner interface. Let me check this...
In the way above, NEXT_TRACK is no longer a trigger op, but it's rather an operational option. This would set a flag something like compr->drain_mode = SND_COMPR_DRAIN_RESTART; while its default is SND_COMPR_DRAIN_STOP. (Or, set compr->next_track_after_drain = true, or whatever.)
So the basic difference would be that stream is never stopped in gapless case. The stream should retain running state maybe trasition to intermediate one for book keeping but then needs to go back to running.
But, should framework really care about that as DSP is doing handling!
-- ~Vinod
participants (2)
-
Takashi Iwai
-
Vinod Koul