[FFmpeg-devel] [PATCH] AST muxer

[jamal]

On 22/11/12 4:02 PM, Paul B Mahol wrote:
>> +typedef struct ASTContext {
> 
> Name it something like ASTMuxContext, it is muxer after all.

Ok.

>> +static int ast_write_header(AVFormatContext *s)
>> +{
>> +    ASTContext *ast = s->priv_data;
>> +    AVIOContext *pb = s->pb;
>> +    AVCodecContext *enc = s->streams[0]->codec;
> 
> There should be check for number of streams, because ast supports only
> one audio stream.

Ok.

>> +    int codec_id;
>> +    double sample = (double)enc->sample_rate / 1000;
>> +    av_log(s, AV_LOG_DEBUG, "(double) sample_rate / 1000: %f\n", sample);
> 
> This 2 lines are of dubious debug usability and should be removed.

The only debug line is the av_log one.
The sample variable is needed to get the loopstart and loopend values (converting milliseconds to samples).

I added that av_log line to easily check the value of said variable if needed, but I can remove it if you think it's not worth it.

>> +
>> +    CHECK_LOOP(start)
>> +    CHECK_LOOP(end)

> Is this really required? Bellow AVOptions should already take care of that.

The AVOptions loopstart and loopend are in milliseconds, not samples.
This macro converts them to samples, and makes sure the result value is not bigger than 0xFFFFFFFF, or negative (Unlikely, but just in case the conversion is big enough to become negative on an int64_t).

>> +
>> +    if(ast->loopstart && ast->loopend && ast->loopstart >= ast->loopend) {
>> +        av_log(s, AV_LOG_ERROR, "Loopend can't be less or equal to loopstart\n");
>> +        return -1;
> 
> AVERROR(EINVAL);

Ok.

>> +    }
>> +
>> +    switch(enc->codec_id) {
>> +    case AV_CODEC_ID_ADPCM_AFC:
>> +        codec_id = 0;
>> +        break;
>> +    case AV_CODEC_ID_PCM_S16BE_PLANAR:
>> +        codec_id = 1;
>> +        break;
>> +    default:
>> +        av_log(s, AV_LOG_ERROR, "Unsupported codec\n");
>> +        return AVERROR_INVALIDDATA;
>> +    }
> 
> You could add table which could then be shared between muxer and demuxer.
> (But first demuxer file should be renamed to astdec)

Ok, I'll resend this patch with this included, unless you prefer a separate patch to rename the demuxer.

>> +
>> +    if(enc->channels != 2 && enc->channels != 4) {
>> +        av_log(s, AV_LOG_ERROR, "Unsupported channel amount: %d\n", enc->channels);
>> +        return AVERROR_INVALIDDATA;
>> +    }
> 
> Being able to test on Wii that different channel count are supported
> would be welcome.

So far every game using the format uses stereo streams and occasionally 4.0 streams. The AST format can support more, but since no game seems to use anything beyond stereo and quad i thought about forcing them.
I can remove the channel limitation if that's better, since the muxer and the demuxers accept and work fine with streams of any amount of channels as far as i tested.

>> +
>> +    ffio_wfourcc(pb, "STRM");
>> +
>> +    ast->size = avio_tell(pb);
>> +    avio_wb32(pb, 0); /* File size minus header */
>> +    avio_wb16(pb, codec_id);
>> +    avio_wb16(pb, enc->bits_per_coded_sample);
> 
> Demuxer does not export this. And it is actually bits per sample for
> output which is always 16 (and not input, where it is 4 for APC and 16 for PCM).

avio_wb16(pb, 0x10) then?

>> +static int ast_write_trailer(AVFormatContext *s)
>> +{
>> +    AVIOContext *pb = s->pb;
>> +    ASTContext *ast = s->priv_data;
>> +    AVCodecContext *enc = s->streams[0]->codec;
>> +    int64_t file_size = avio_tell(pb);
>> +    int64_t samples = (file_size - 64 - (32 * enc->frame_number)) / enc->block_align;
> 
> Does this work for APC too? You could use -c copy and then duration of
> output should match one of input.

It should, as long as enc->block_align is not zero. To be sure, can you upload the ADPCM AFC sample somewhere so i can check this?
In any case, I can add a check for enc->block_align and if it's zero make it (enc->bits_per_coded_sample * enc->channels) >> 3

And what do you mean with using -c copy?

>> +        avio_seek(pb, file_size, SEEK_SET);
> 
> SEEK_END

Ok.

> 
> That's all (for now).

Thanks. I'll send a new patch after you answer the concerns above.