[igt-dev] [PATCH i-g-t v4 1/5] tests/kms_chamelium: add dp-audio test

[Ser, Simon]

On Wed, 2019-04-17 at 15:17 +0300, Martin Peres wrote:
> > > > +	if (dump_fd >= 0) {
> > > > +		close(dump_fd);
> > > > +		if (streak == MIN_STREAK) {
> > > > +			/* Test succeeded, no need to keep the captured data */
> > > > +			unlink(dump_path);
> > > > +		} else
> > > > +			igt_debug("Saved captured audio data to %s\n", dump_path);
> > > > +		free(dump_path);
> > > > +	}
> > > > +
> > > > +	free(recv);
> > > > +	free(buf);
> > > > +	free(channel);
> > > > +
> > > > +	ok = chamelium_stream_stop_realtime_audio(stream);
> > > > +	igt_assert(ok);
> > > > +
> > > > +	audio_file = chamelium_stop_capturing_audio(data->chamelium,
> > > > +						    port);
> > > > +	if (audio_file) {
> > > > +		igt_debug("Audio file saved on the Chamelium in %s\n",
> > > > +			  audio_file->path);
> > > > +		chamelium_destroy_audio_file(audio_file);
> > > > +	}
> > > 
> > > I would suggest to only dump this file on failure, not when having a
> > > success.
> > 
> > 1. We can't decide this after-the-fact: we can only decide whether we
> >    dump or not before starting the capture.
> > 2. There are two kinds of audio dumps: local (on the DUT, see dump_fd) 
> >    and remote (on the Chamelium, see the last param of 
> >    chamelium_start_capturing_audio). If the file has been dumped on the
> >    Chamelium, chamelium_stop_capturing_audio will return the audio file
> >    details. It's sometimes useful to enable Chamelium dumps for
> >    debugging purposes.
> 
> Of course! Sorry for the confusion! Where are we dumping the generated
> and received WAVs when the test is failing then? Is that a TODO?

So, this is done a little earlier (see the quoted code above). We do
also dump audio data for tests that succeed, but unlink the file in
that case. This allows us not to keep all of the captured data in
memory (dumps are generally worth a couple of MiB) and to keep the code
simple (no dynamic memory allocation). But honestly I'm not feeling
strongly about this and I'm open to change it.