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

[Alexis Ballier]

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.

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...

Alexis.