[FFmpeg-devel] [PATCH] avformat/mov: mov_build_index: pick the bigger sample_size of the 2

[Alex Sukhanov]

On Fri, May 24, 2013 at 10:15 AM, Michael Niedermayer <michaelni at gmx.at>wrote:

> On Thu, May 23, 2013 at 11:21:26PM -0700, Alex Sukhanov wrote:
> > On Thu, May 23, 2013 at 7:00 PM, Michael Niedermayer <michaelni at gmx.at
> >wrote:
> >
> > > On Thu, May 23, 2013 at 06:09:06PM -0700, Alex Sukhanov wrote:
> > > > On Thu, May 23, 2013 at 3:48 PM, Michael Niedermayer <
> michaelni at gmx.at
> > > >wrote:
> > > >
> > > > > On Thu, May 23, 2013 at 02:23:18PM -0700, Alex Sukhanov wrote:
> > > > > > On Thu, May 23, 2013 at 1:13 PM, Michael Niedermayer <
> > > michaelni at gmx.at
> > > > > >wrote:
> > > > > >
> > > > > > > Fixes Ticket2605
> > > > > > >
> > > > > > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > > > > > > ---
> > > > > > >  libavformat/mov.c |    2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > > > > > index d5e1f89..9497884 100644
> > > > > > > --- a/libavformat/mov.c
> > > > > > > +++ b/libavformat/mov.c
> > > > > > > @@ -2018,7 +2018,7 @@ static void mov_build_index(MOVContext
> *mov,
> > > > > > > AVStream *st)
> > > > > > >                  }
> > > > > > >                  if (keyframe)
> > > > > > >                      distance = 0;
> > > > > > > -                sample_size = sc->alt_sample_size > 0 ?
> > > > > > > sc->alt_sample_size : sc->sample_sizes[current_sample];
> > > > > > > +                sample_size = sc->alt_sample_size > 0 ?
> > > > > > > FFMAX(sc->alt_sample_size, sc->sample_size) :
> > > > > > > sc->sample_sizes[current_sample];
> > > > > > >                  if (sc->pseudo_stream_id == -1 ||
> > > > > > >                     sc->stsc_data[stsc_index].id - 1 ==
> > > > > > > sc->pseudo_stream_id) {
> > > > > > >                      AVIndexEntry *e =
> > > > > > > &st->index_entries[st->nb_index_entries++];
> > > > > > > --
> > > > > > > 1.7.9.5
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > ffmpeg-devel mailing list
> > > > > > > ffmpeg-devel at ffmpeg.org
> > > > > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > > >
> > > > > >
> > > > > >
> > > > > > Hi Michael,
> > > > > >
> > > > > > Thanks for looking on this problem. I just think that your fix
> > > > > potentially
> > > > > > doesn't cover all cases.
> > > > > > With your fix now we able to extract audio if STSZ sample_size <
> real
> > > > > > sample_size, as we have it in my sample file:
> > > > > > STSZ::sample_size = 1
> > > > > > real sample size = 4
> > > > > >
> > > > >
> > > > > > But what if STSZ::sample_size = 100
> > > > > > real sample size = 4?
> > > > >
> > > > > then the value 100 will be used
> > > > >
> > > > >
> > > > > >
> > > > > > Don't you think that we should revert
> > > > > >
> > > > >
> > >
> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=50059bde77674977d9134f3c1151a63cb7a2391c
> > > > >
> > > > > If you revert the Fix for #522 then the sample from #522 will fail
> > > > >
> > > > >
> > > > >
> > > > Well, I'm looking on noaudio.mp4 (from #522) and see that
> > > > - Audio pcm_mulaw
> > > > - Track Timescale 8000
> > > > - num_channels 1
> > > > - bits_per_sample 8
> > > > - Sample duration is variable: 639, 640 640, 641, 648
> > > > - Sample size (STSZ) = 640 for each sample, what is incorrect. As
> long as
> > > > sample size should be equal to sample duration in this configuration.
> > > >
> > > > BTW, I found that $mplayer -demuxer mov plays both (noaudio.mp4 and
> my
> > > > sample) correctly.
> > > >
> > > > So I guess that PCM audio recovering should be implemented in
> different
> > > way.
> > >
> > > Do you have a sample that fails with the patch?
> > >
> >
> > I uploaded audio_silence_after_ffmpeg_upgrade_100.mov on ftp
> > upload.ffmpeg.org
> > To my mind it should fail for this patch
>
> let me rephrase my question
> Do you have a quicktime file that is valid / that can be played back
> with some player AND this file fails with the patch.
> You edited the file and changed 1 byte, what makes you think this file
> is then still a valid quicktime file ?
> Or how is the behavior with such editted file related to the validity
> of a patch.
>
> Or can you explain what problem you see with the patch?
>
>
Yes, you asked me to provide such sample - I did it. just modified one
byte.
I suspect that your patch doesn't work well with such file. Unfortunately I
don't have chance right now to download you patch and check that.
If you could try your patch on both files (STSZ sample size = 1, and 255)
that would be great.

You added FFMAX, by this you added support of stsz->sample_size <
real_sample_size. Why only that one?
Why would you like to support size 1, but not 255?

Before patch
http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=50059bde77674977d9134f3c1151a63cb7a2391c
you supported both files (size 1 and 255 in stsz)

You know this code much better than me, but I think that issue #522 should
be fixed slightly different and
50059bde77674977d9134f3c1151a63cb7a2391c<http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=50059bde77674977d9134f3c1151a63cb7a2391c>
should
be reverted.


> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>