[FFmpeg-devel] [PATCH] avcodec: add ATRAC Advanced Lossless decoders

[Michael Niedermayer]

On Tue, Jan 31, 2017 at 06:29:03PM +0100, Paul B Mahol wrote:
> On 1/31/17, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Tue, Jan 31, 2017 at 04:17:19PM +0100, Paul B Mahol wrote:
> >> On 1/31/17, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> > On Sun, Jan 29, 2017 at 06:39:21PM +0100, Paul B Mahol wrote:
> >> >> Only lossy part is decoded for now.
> >> >>
> >> >> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> >> >> ---
> >> > [...]
> >> >>  static int oma_read_packet(AVFormatContext *s, AVPacket *pkt)
> >> >>  {
> >> >>      OMAContext *oc  = s->priv_data;
> >> >> -    AVStream *st    = s->streams[0];
> >> >> -    int packet_size = st->codecpar->block_align;
> >> >> -    int byte_rate   = st->codecpar->bit_rate >> 3;
> >> >> -    int64_t pos     = avio_tell(s->pb);
> >> >> -    int ret         = av_get_packet(s->pb, pkt, packet_size);
> >> >> -
> >> >> -    if (ret < packet_size)
> >> >> -        pkt->flags |= AV_PKT_FLAG_CORRUPT;
> >> >> -
> >> >> -    if (ret < 0)
> >> >> -        return ret;
> >> >> -    if (!ret)
> >> >> -        return AVERROR_EOF;
> >> >> -
> >> >> -    pkt->stream_index = 0;
> >> >> -
> >> >> -    if (pos >= oc->content_start && byte_rate > 0) {
> >> >> -        pkt->pts =
> >> >> -        pkt->dts = av_rescale(pos - oc->content_start,
> >> >> st->time_base.den,
> >> >> -                              byte_rate *
> >> >> (int64_t)st->time_base.num);
> >> >> -    }
> >> >> -
> >> >> -    if (oc->encrypted) {
> >> >> -        /* previous unencrypted block saved in IV for
> >> >> -         * the next packet (CBC mode) */
> >> >> -        if (ret == packet_size)
> >> >> -            av_des_crypt(oc->av_des, pkt->data, pkt->data,
> >> >> -                         (packet_size >> 3), oc->iv, 1);
> >> >> -        else
> >> >> -            memset(oc->iv, 0, 8);
> >> >> -    }
> >> >> -
> >> >> -    return ret;
> >> >> +    return oc->read_packet(s, pkt);
> >> >>  }
> >> >
> >> > moving this into read_packet() could be done in a seperate patch
> >> >
> >> >
> >> >>
> >> >>  static int oma_read_probe(AVProbeData *p)
> >> >> @@ -491,8 +571,14 @@ static int oma_read_seek(struct AVFormatContext
> >> >> *s,
> >> >>                           int stream_index, int64_t timestamp, int
> >> >> flags)
> >> >>  {
> >> >>      OMAContext *oc = s->priv_data;
> >> >> -    int64_t err = ff_pcm_read_seek(s, stream_index, timestamp,
> >> >> flags);
> >> >> +    AVStream *st = s->streams[0];
> >> >> +    int64_t err;
> >> >> +
> >> >> +    if (st->codecpar->codec_id == AV_CODEC_ID_ATRAC3PAL ||
> >> >> +        st->codecpar->codec_id == AV_CODEC_ID_ATRAC3AL)
> >> >
> >> >> +        return -1;
> >> >
> >> > should be a AVERROR code
> >>
> >> This is not error, it makes seeking possible, using other error codes
> >> is bad idea.
> >
> > It would be better to use a named identifier than a litteral number
> > normally we use AVERROR_*, i would argue that oma_read_seek() did not
> > seek so it didnt succeed but if you prefer this to be called something
> > else than AVERROR_* sure iam perfectly fine with what you prefer.
> >
> > It was the use of a litteral number (which is also undocumented for
> > read_seek() except by code returning -1) that i wanted to comment on
> 
> -1 means use generice seeking.

I think any negative return results in that unless disabled but its
long ago that i worked on this code.

-1 is kind of bad because it is not descriptive, a reader doesnt know
what that does also -1 == AVERROR(EPERM) here
so it cannot be distinguished from that error

i think we should add and use soemthing like
return AVERROR_USE_GENERIC_SEEK;
return FF_USE_GENERIC_SEEK;
return FALLBACK_TO_GENERIC_SEEK;
return WHATEVER_ELSE_ONE_LIKES_TO_CALL_IT;



[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170131/ed36b875/attachment.sig>