[FFmpeg-devel] [PATCH] AST muxer

[Paul B Mahol]

On 11/22/12, jamal <jamrial at gmail.com> wrote:
> 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.

Using double for this is not portable. Please use AVRational and av_rescale*.
>
>>> +
>>> +    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.

Separate. (Note that I do not know which solution table or current
code bloat less; both in source code and in binary).
>
>>> +
>>> +    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.

You get that from wiki entry? Wikis are not meant to be definite truth.
> 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?

16
>
>>> +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

You can look in source code how samples are stored in ADPCM AFC.

It is 9 bytes for each 16 samples per channel. Doing 9 : 32 compression ratio.
>
> And what do you mean with using -c copy?

That is used with ffmpeg to copy packets from input to output - just
remuxing without any encoding involved.
>
>>> +        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.