bsutton / sounds

Flutter plugin for sound. Audio recorder and player.
GNU Lesser General Public License v3.0
78 stars 25 forks source link

Wrap Codec in a MediaFormat #5

Closed bsutton closed 4 years ago

bsutton commented 4 years ago

I've been looking into the issues around #344 (re-architect codec support) and have come to the realisation that we have a number of problems with how we are using the Codec class.

Reference material: https://developer.android.com/guide/topics/media/media-formats

The Codec class has morphed into an object that attempts to describe both the Codec and the media container.

This appears problematic as the Codec is not able to fully describe a media's format and the class name is misleading as it describes more than just a Codec.

I'm proposing that we create a new class MediaFormat.

class MediaFormat
{
 Codec codec;
 MediaContainer container;
int sampleRate = 16000,
int numChannels = 1,
int bitRate = 16000,
Quality quality = Quality.low,
}

Note: I'm uncertain whether quality belongs here, its taken from the SoundRecorder.record method.

The MediaFormat would replace Codec in the public api so when creating a Track you would now use something like:

var format = MediaFormat.default;
var track = Track.fromPath(path, format);

var format = MediaFormat(Codecs.byName('mpeg'), MediaContainer.byName('mpeg'));
format.sampleRate = 1000;
format.numChannels = 2;

track  = Track.fromBuffer(buffer, format);

I believe this method has a number of advantages: 1) we are now accurately describing the users requested media format. 2) when doing codec conversions we would use the MediaFormat rather than just the Codec. This allows conversions to fully describe the desired target format. (e.g. stereo to mono, down sampling etc). 3) We change the SoundRecorder.record method to take a MediaFormat which makes the api consistent. 4) we future proof the api as MediaFormat accurately describes the format which means we are less likely to have to change the api. With the current implementation this is highly likely as the current Codec is a enum which means we can't add additional properties.

The down side is that it probably makes our life harder as we need to deal with users passing formats that are not supported or inconsistent. Inconsistency is perhaps a problem with the SoundPlayer as it would essentially ignore the properties. The flip side is that I can't think of why this would be an actual problem. We also make the users life slightly harder as it takes more lines of code to describe a MediaFormat than a Codec. Having a number of default MediaFormats would mitigate this problem.

Thoughts?

bsutton commented 4 years ago

One option to consider is that we only support native codecs for playback and recording. Any other codecs must be converted to a native codec before passing them to Sounds.

This would mean that for playback the MediaFormat is meaningless as at the most we would need to identify a codec. For recording the meta data might still be useful but then we could just continue passing the channels/sampleRate etc directly to the ctor.

If thats the case we would only need the media format for the codec conversion library.

bsutton commented 4 years ago

So I think that for playback we just require a codec and it must be a native coded (not conversion support). Any conversions must be done before we pass the track to playback. The SoundPlayerUI widget will support codec conversion but it will do the conversion before it calls into the SoundPlayer api. Widget support of conversion is necessary to provide a decent 'progress' indicator as the conversion needs to be part of the progression towards playback.

For recording we are going to require a MediaFormat to fully describe the recording properties. The SoundRecorder api however will only support native codecs and will throw a CodecUnsupportedException should an invalid codec be provided.

bsutton commented 4 years ago

One thought is if we should remove support for any stream preparation from the SoundPlayer and SoundRecorder api classes.

This would mean that a Track loaded from an asset would have to be pre-preparded before calling into SoundPlayer. We already support preparation of these type of track storage types so what advantage to we get by removing them? 1) simplified code. 2) the sound player api will always start playing immediately.

The downside is that we make the code uglier to user as they now have to call prepareStream before passing it to the playback api.

On balance I think we should leave this bit alone.

bsutton commented 4 years ago

Do we even need a MediaFormat for the SoundPlayer. Codec is not used in the java or ios code at all. In fact we should move the isCodecSupported logic into dart code as its just a map.

We need media formats to describe what is to be recorded and for doing transcoding. So the question is why are we passing codecs around? The MediaFormat could be constrained to the transcoding libraries and directly passed to the recording api.

The nice thing about the MediaFormat is that it allowed us to pass complete details around in a track. This allows us to simply pass a track into a transcoding library. But there is probably an argument that the transcoding libraries could just expect the mediaformat to be passed as well.

There are also questions about what we need to pass to the transcoding libraries.

When recording we may not be able to guarantee the mediaformat of the recording (is this true or does the recording just fail if we passed invalid values?). If we can end up with an media format that doesn't not match the requested format then we should return the actual format. Hmm, its probably easier to just throw an exception if we can't do exactly what is requested as this will make things simpler all round.

So we change track so that it doesn't carry a media format. If you want to transcode after recording you are going to have to carry the media format around separately. Or could we make the media format on a track optional so that it can be passed as a single object. Maybe if you have a track and request the media format then we could go an inspect the track? No this would require ffmpeg again. The sounds_codec package could provide this but the core package should not.

So in the core package the MediaFormat seems to only have relavance when recording. Except that the Widgets will offer transcoding (via callbacks into the sounds_codec package if it is loaded) so the widgets will need to know about MediaFormats. So the SoundPlayer api will always ignore the MediaFormat. SoundRecorder api will use it to define the recording format and the Widgets will rely heavily on it.

Should the SoundRecorder widget do transcoding? The advantage is an out of the box UI that does transcoding. The disadvantage is that we are imposing a UI during transcoding. Of course the user can requests a MF that doesn't required transcoding so the user would never see the SoundRecorderUI widgets transcoding interface.

Conclusion:

bsutton commented 4 years ago

Remove the codec from the platform call to start player. Remove the is support codec logic from the platform code.

bsutton commented 4 years ago

done.