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

[Marton Balint]

On Tue, 27 Feb 2018, Marton Balint wrote:

>
> On Thu, 22 Feb 2018, Paul B Mahol wrote:
>
>> On 2/22/18, Tomas Haerdin <tjoppen at acc.umu.se> wrote:
>>> ons 2018-02-21 klockan 23:06 +0100 skrev Marton Balint:
>>>> On Wed, 21 Feb 2018, Tomas Haerdin wrote:
>>>>
>>>> > loer 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
>>
>> YOu are lazy and evil! People please ignore him!
>
> I plan to apply the series soon, unless there are other comments :)

Applied.

Regards,
Marton