[Ffmpeg-devel] [PATCH] new mov_read_packet, mov_read_seek

[Michael Niedermayer]

Hi

On Fri, Jul 07, 2006 at 12:28:38PM +0200, Baptiste Coudurier wrote:
> Hi
> 
> Here is a new mov_read_packet and mov_read_seek functions. That should
> fix seek in non interleaved mov, demux correctly adpcm.
> It computes pkt->duration, mainly for stream copy, to later compute
> stts in muxing, since it cannot be done anywhere else (AAC for example)
> 
> Finally, I removed the old obsolete messy code, is it fine ?

ok


> 
> It does not yet support edit list. Im working on it though. Just have a
> few questions, Im wondering if we really should not demux those "not
> displayed packets", I would be more in favor for setting a flag that
> mention that the packet must not be displayed, for 2 reasons:
> - If the start time requested is not a key frame, it will not be decoded
> properly.
> - It would be useful to transcode those packets but keeps the edit list
> if you later want to re-edit the file.
> 
> What do you guys familiar with mov think about it ?

edit lists .... well
how should that be implemented?
there are 2 ways
1. output stuff exactly as the edit lists say 
2. output stuff completely ignoring the edit lists and exporting the
   information from the edit lists in a nice way, maybe a AVEditList
   or so, so that the user application can decide what to do, keep in
   mind that edit lists and such could reorder and duplicate stuff so
   a simple dont-display flag isnt enough

if now the mov-edit-lists ignore keyframes and contain stuff like
"display from frame 100 to 150" while frame 100 is no keyframe
then 1. is not cleanly possible anyway


[...]
>              switch (st->codec->codec_id) {
> +            case CODEC_ID_PCM_S8:
> +            case CODEC_ID_PCM_U8:
> +                if (st->codec->bits_per_sample == 16) {
> +                    st->codec->codec_id = CODEC_ID_PCM_S16BE;
> +                    sc->sample_size = 2 * st->codec->channels;
> +                } else
> +                    sc->sample_size = 1 * st->codec->channels;
> +                break;
>              case CODEC_ID_PCM_S16LE:
>              case CODEC_ID_PCM_S16BE:
> -                if (st->codec->bits_per_sample == 8)
> +                if (st->codec->bits_per_sample == 8) {
>                      st->codec->codec_id = CODEC_ID_PCM_S8;
> -                st->codec->bit_rate = st->codec->sample_rate * 8;
> +                    sc->sample_size = 1 * st->codec->channels;
> +                } else
> +                    sc->sample_size = 2 * st->codec->channels;
>                  break;
> -            case CODEC_ID_PCM_U8:
> -                if (st->codec->bits_per_sample == 16)
> -                    st->codec->codec_id = CODEC_ID_PCM_S16BE;
> -                st->codec->bit_rate = st->codec->sample_rate * 8;
> +            case CODEC_ID_PCM_S24BE:
> +            case CODEC_ID_PCM_S24LE:
> +                sc->sample_size = 3 * st->codec->channels;
>                  break;
> +            case CODEC_ID_PCM_S32BE:
> +            case CODEC_ID_PCM_S32LE:
> +                sc->sample_size = 4 * st->codec->channels;
> +                break;
> +            case CODEC_ID_PCM_MULAW:
> +            case CODEC_ID_PCM_ALAW:
> +                sc->sample_size = 1 * st->codec->channels;
> +                break;
>              case CODEC_ID_AMR_WB:
>                  st->codec->sample_rate = 16000; /* should really we ? */
>                  st->codec->channels=1; /* really needed */

could you add a little function to libavcodec/utils.c :
something like
av_get_bits_per_sample(CodecID codec_id){
    switch(codec_id;
    case CODEC_ID_PCM_S16LE:
    case CODEC_ID_PCM_S16BE:
        return 16;
    ...
    default:
        return 0;
}

and use it, there are so many places in libav* which need this that its
starting to hurt to see this in various varations over and over ...

[...]


> +    /* compute duration */
> +    assert(sc->stts_data[sc->sample_to_time_index].duration % sc->time_rate == 0);
> +    pkt->duration = sc->stts_data[sc->sample_to_time_index].duration / sc->time_rate;

i think this is wrong, if this is the dts difference then its not the real
duration, AVPacket.duration must be the real duration, which is the pts
difference

for example:
         IPBBP
dts:     01356
pts:     16359
duration:2321


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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is