Thanks for your reply.
If a track id of the test clip does not appear in its ‘moov’, the do/while
loop cannot exit indeed.
The purpose of this patch is to solve AV_NOPTS_VALUE in the fragment info
structure. Because the original structures is oriented to ‘moof’ boxes in
file. I tried to organize fragment info according to media tracks.
Since the fragment structures has been changed, it fixes two other issues.
One is #7572, which missed the last ‘sidx’ and ‘moof’.
The other is “mov->fragment.stsd_id == 1” in function cenc_filter(). Mov
muxer read all ‘moof’ while reading header. So the current fragment points
to the last ‘moof’. When we read packets/samples from the beginning, the
sample (belongs to the first ‘moof’) and the current fragment info do not
match.
I don’t mind waiting for other options. And we already have a quick fix
aa25198f1b925a464bdfa83a98476f. Though, I will still update the patch to
resolve the endless loop problem.
BTW, I have’t modified other commit messages. The frag index variable is
changed in isom.h.
Thanks again, and sorry for late response.
Regards,
Charles.
On Wed, Feb 27, 2019 at 9:39 PM Michael Niedermayer <mich...@niedermayer.cc>
wrote:
> On Mon, Feb 25, 2019 at 07:07:27PM +0800, C.H.Liu wrote:
> > Would you mind share your input file? I still can’t reproduce the OOM.
>
> i belive i cannot share this sample, but the patch really needs to be
> fully reviewed not just one bug that was found by chance fixed otherwise
> we might be missing other bugs ...
>
>
> >
> >
> > I have tried several types of mp4.
> >
> > I also tried to create a file like yours. According to the snapshot,
> seems
> > it has no ‘sidx’ box and enable ‘use_mfra_for’.
> >
> > Massif didn’t report update_frag_index() function. I didn’t see an extra
> > heap as big as here, either.
>
> looking at the sample that triggers the OOM the do/while loop you add
> to update_frag_index() is entered and never exits while doing allocations
> in it.
>
> also the patch as is is not ok. You for example have a list of unrelated
> changes in the commit message. Each should be in its own patch
> also changes that are functional should be splited from non functional
> changes. For example here:
>
> // If moof_offset already exists in frag_index, return index to it
> - index = search_frag_moof_offset(&c->frag_index, offset);
> - if (index < c->frag_index.nb_items &&
> - c->frag_index.item[index].moof_offset == offset)
> + index = search_frag_moof_offset(frag_index, offset);
> + if (index < frag_index->nb_items &&
> + frag_index->item[index].moof_offset == offset) {
> + frag_index->current = index;
> return index;
> + }
>
> you change frag_index to a local variable this is non functional and
> should not be in a patch that does a functional change ideally.
> Now sure if a functional change is trivial and clear such things are
> sometimes done together but this patch is not clear, at least its not
> clear to me ...
>
> thanks
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I do not agree with what you have to say, but I'll defend to the death your
> right to say it. -- Voltaire
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
