[FFmpeg-devel] [PATCH 02/10] avformat/mxfdec: fix essence_offset calculation

[Tomas Härdin]

ons 2018-02-21 klockan 23:06 +0100 skrev Marton Balint:
> On Wed, 21 Feb 2018, Tomas Härdin wrote:
> 
> > lör 2018-02-17 klockan 22:45 +0100 skrev Marton Balint:
> > > The reference point for a KAG is the first byte of the key of a Partition Pack.
> > > 
> > > Fixes ticket #2817.
> > > Fixes ticket #5317.
> > > 
> > > > Signed-off-by: Marton Balint <cus at passwd.hu>
> > > 
> > > ---
> > >  libavformat/mxfdec.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index fcae863ef4..95767ccba4 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -2875,8 +2875,8 @@ static int mxf_read_header(AVFormatContext *s)
> > >                   *       for OPAtom we still need the actual essence_offset though (the KL's length can vary)
> > >                   */
> > >                  int64_t op1a_essence_offset =
> > > -                    round_to_kag(mxf->current_partition->this_partition +
> > > -                                 mxf->current_partition->pack_length,       mxf->current_partition->kag_size) +
> > > +                    mxf->current_partition->this_partition +
> > > +                    round_to_kag(mxf->current_partition->pack_length,       mxf->current_partition->kag_size) +
> > >                      round_to_kag(mxf->current_partition->header_byte_count, mxf->current_partition->kag_size) +
> > >                      round_to_kag(mxf->current_partition->index_byte_count,  mxf->current_partition->kag_size);
> > 
> > This seems like a rather elemental miscalculation, that I wrote even. I
> > took a look at S377m, it had this to say:
> > 
> > > The first gridline in any partition is the first byte of the key of
> > > the partition pack that defines that
> > > partition.
> > 
> > Each partition starts at ThisPartition (plus run-in) so this patch
> > should be correct. What's perhaps more suspect is the calculation
> > itself. It should *always* be possible to locate where Op1a essence
> > starts, by scanning to the first essence KLV. MXF is flexible enough
> > that having some sketchy calculation for where it *might* be is
> > probably not correct. One is free to insert any amount of padding
> 
> The next patch more or less handles this by checking for a system item 
> key and explicitly setting the offset if that is found. An essence alone 
> however might not be the first essence, it is also possible that we 
> already skipped an unknown essence key, or an unknown system item key, so 
> I decided to keep the code as is if the first recognized essence is not a 
> system item.

Sounds reasonable I guess. I'm going to reiterate that I consider
continuing to maintain mxfdec is a mistake. A wrapper around
bmxlib/libMXF would be much easier to maintain

/Tomas