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