Re: [Qemu-devel] [PATCH v4 04/14] audio: -audiodev command line option basic implementation

[Zoltán Kővágó]

On 2019-01-29 16:56, Markus Armbruster wrote:
> "Kővágó, Zoltán" <address@hidden> writes:
> 
>> Audio drivers now get an Audiodev * as config paramters, instead of the
>> global audio_option structs.  There is some code in audio/audio_legacy.c
>> that converts the old environment variables to audiodev options (this
>> way backends do not have to worry about legacy options).  It also
>> contains a replacement of -audio-help, which prints out the equivalent
>> -audiodev based config of the currently specified environment variables.
>>
>> Note that backends are not updated and still rely on environment
>> variables.
>>
>> Also note that (due to moving try-poll from global to backend specific
>> option) currently ALSA and OSS will always try poll mode, regardless of
>> environment variables or -audiodev options.
>>
>> Signed-off-by: Kővágó, Zoltán <address@hidden>
>> ---
> [...]
>> diff --git a/audio/audio.c b/audio/audio.c
>> index ce8e6ea8c2..b37c245a8a 100644
>> --- a/audio/audio.c
>> +++ b/audio/audio.c
> [...]
>> @@ -2129,3 +1866,145 @@ void AUD_set_volume_in (SWVoiceIn *sw, int mute, 
>> uint8_t lvol, uint8_t rvol)
>>          }
>>      }
>>  }
>> +
>> +static void audio_validate_per_direction_opts(
>> +    AudiodevPerDirectionOptions **pdo, bool *has_pdo, Error **errp)
>> +{
>> +    if (!*has_pdo) {
>> +        *pdo = g_malloc0(sizeof(AudiodevPerDirectionOptions));
>> +        *has_pdo = true;
>> +    }
>> +
>> +    if (!(*pdo)->has_fixed_settings) {
>> +        (*pdo)->has_fixed_settings = true;
>> +        (*pdo)->fixed_settings = true;
>> +    }
>> +    if (!(*pdo)->fixed_settings &&
>> +        ((*pdo)->has_frequency || (*pdo)->has_channels || 
>> (*pdo)->has_format)) {
>> +        error_setg(errp,
>> +                   "You can't use frequency, channels or format with 
>> fixed-settings=off");
>> +        return;
>> +    }
>> +
>> +    if (!(*pdo)->has_frequency) {
>> +        (*pdo)->has_frequency = true;
>> +        (*pdo)->frequency = 44100;
>> +    }
>> +    if (!(*pdo)->has_channels) {
>> +        (*pdo)->has_channels = true;
>> +        (*pdo)->channels = 2;
>> +    }
>> +    if (!(*pdo)->has_voices) {
>> +        (*pdo)->has_voices = true;
>> +        (*pdo)->voices = 1;
>> +    }
>> +    if (!(*pdo)->has_format) {
>> +        (*pdo)->has_format = true;
>> +        (*pdo)->format = AUDIO_FORMAT_S16;
>> +    }
>> +}
>> +
>> +static void audio_validate_opts(Audiodev *dev, Error **errp)
>> +{
>> +    Error *err = NULL;
>> +
>> +    audio_validate_per_direction_opts(&dev->in, &dev->has_in, &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +
>> +    audio_validate_per_direction_opts(&dev->out, &dev->has_out, &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +
>> +    if (!dev->has_timer_period) {
>> +        dev->has_timer_period = true;
>> +        dev->timer_period = 10000; /* 100Hz -> 10ms */
>> +    }
>> +}
> 
> Drive-by question: this validates just the generic part of Audiodev gets
> validated, not the driver-specific part.  Is there nothing to validate?
> Does it happen somewhere else?

Driver specific validation/initialization happens in each driver's init
function (they're added in the upcoming patches).

>> +
>> +void audio_parse_option(const char *opt)
>> +{
>> +    AudiodevListEntry *e;
>> +    Audiodev *dev = NULL;
>> +
>> +    Visitor *v = qobject_input_visitor_new_str(opt, "driver", &error_fatal);
>> +    visit_type_Audiodev(v, NULL, &dev, &error_fatal);
>> +    visit_free(v);
>> +
>> +    audio_validate_opts(dev, &error_fatal);
>> +
>> +    e = g_malloc0(sizeof(AudiodevListEntry));
>> +    e->dev = dev;
>> +    QSIMPLEQ_INSERT_TAIL(&audiodevs, e, next);
>> +}
>> +
>> +void audio_init_audiodevs(void)
>> +{
>> +    AudiodevListEntry *e;
>> +
>> +    QSIMPLEQ_FOREACH(e, &audiodevs, next) {
>> +        audio_init(e->dev);
>> +    }
>> +}
> 
> Your use of QAPI here looks good to me.
> 
> [...]
>