[FFmpeg-devel] [PATCH 04/11] libavformat/mxfdec.c: Try to increment current edit before rejecting a klv that spans onto next edit unit.

[Tomas Härdin]

On Thu, 2015-10-22 at 19:47 +0200, Alexis Ballier wrote:
> On Wed, 21 Oct 2015 23:45:07 +0200
> Tomas Härdin <tomas.hardin at codemill.se> wrote:
> 
> > On Wed, 2015-10-21 at 18:00 +0200, Alexis Ballier wrote:
> > > Some files such as those from tickets #2817 & #2776 claim to have
> > > constant edit unit size but, in fact, have some of them that are
> > > smaller. This confuses the demuxer that tries to infer the current
> > > edit unit from the position in the file. By trying to increment the
> > > current edit unit before rejecting the packet for this reason, we
> > > try to make it fit into its proper edit unit, which fixes demuxing
> > > of those files while preserving the check for misprobed OpAtom
> > > files. Seeking is not accurate but the files provide no way to
> > > properly find the relevant edit unit.
> > > 
> > > Fixes tickets #2817 & #2776.
> > > ---
> > >  libavformat/mxfdec.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index 593604e..526eca6 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -2956,6 +2956,18 @@ static int
> > > mxf_read_packet_old(AVFormatContext *s, AVPacket *pkt) next_ofs =
> > > mxf_set_current_edit_unit(mxf, klv.offset); 
> > >              if (next_ofs >= 0 && next_klv > next_ofs) {
> > > +                /* Samples from tickets #2817 and #2776 claim to
> > > have
> > > +                 * constant edit unit size. However, some of them
> > > are smaller.  
> > 
> > What does "them" refer to here? The edit units or the KLVs?
> > 
> > > +                 * Just after those smaller edit units,  
> > 
> > Right, the edit units. Maybe rework the grammar slightly.
> > 
> > > +                 * Just after those smaller edit units, klv.offset
> > > is still in
> > > +                 * the same edit unit according to the
> > > computations from the
> > > +                 * constant edit unit size. If the klv finishes
> > > after, the next
> > > +                 * check would truncate the packet and prevent
> > > proper demuxing.
> > > +                 * Try to increment the current edit unit before
> > > doing that. */  
> > 
> > Let's see if I understand this correctly. For say EUBC = 10, there can
> > still be KLVs that are some size larger than 10, but smaller than
> > 2*EUBC = 20? So that the next edit unit would extend past the end of
> > the KLV, and thus be bogus?
> > 
> >       KLV: |header|-------------------|header|--------------|
> > Edit unit:        |0123456789|bogus<10|      |0123456789|bgs|
> > 
> > IIRC with MXF the bogus parts should still count as part of the
> > essence stream. Maybe I'm missing something.
> 
> It's simpler than that, and if you don't understand then the comment
> likely needs improving :) let's see:
> 
> H = header, V = video, A,B,C = audio tracks, F = fill item
> 
> mxf file defines a proper edit unit, with EUBC = 10 to be something
> like:
> 
> 1234567890
> HVVVAFBFCF
> 
> now, in the samples, in some edit units, video is shorter; mxf spec
> says it should be padded by fill items, but they're not and look like:
> 
> 1234567890
> HVAFBFCF
> 
> when continuing to read, we have:
> 
> 12345678901234567890
> HVAFBFCFHVVVAFBFCF
> | eu 1   || eu 2   |
> 
> as you can see, 2nd video packet is still in the first edit unit
> according to EUBC, and extends to next one.

Ah, that makes it a lot clearer :)

> that's what the patch is about: try to increment edit unit before
> rejecting the packet.
> 
> in 'MXF_DVCAM_not_demuxable.mxf', those smaller video packets seem to
> correspond to a black frame inserted between two scenes.
> 
> I've tried hard to get something better, but nothing seemed to work
> properly; best other option I had was to increment edit unit when
> seeing a system item, which worked but broke tests and in which I'm not
> so confident it won't break with other broken files...

Yeah, breaking existing tests is obviously not OK. But increasing
current_edit_unit like that seems a bit too suspect.

What your patch seems to end up doing with that
max_set_current_edit_unit() call is call mxf_edit_unit_absolute_offset()
like:

	mxf_edit_unit_absolute_offset(mxf, t, mxf->current_edit_unit + 1, NULL, &next_ofs, 0)

Maybe you could make use of just that function call (with
mxf->current_edit_unit + *2*), instead of potentially messing
current_edit_unit up for some corner cases..

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151025/465738e2/attachment.sig>