[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 Sun, 2015-10-25 at 21:43 +0100, Tomas Härdin wrote:
> 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..

Any progress on this? More information needed perhaps?

/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/20151107/b36abc58/attachment.sig>