| Message ID | 20180217214538.24255-2-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | e9b0e42e773d4a1c9575c75e95688faca7ce4e06 |
| Headers | show |
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@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 Of course this entire patchset would be unnecessary if we just used bmxlib instead. /Tomas
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@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. Regards, Marton
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@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
On 2/22/18, Tomas Haerdin <tjoppen@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@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!
On Thu, 22 Feb 2018, Paul B Mahol wrote: > On 2/22/18, Tomas Haerdin <tjoppen@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@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 :) Thanks, Marton
On Tue, 27 Feb 2018, Marton Balint wrote: > > On Thu, 22 Feb 2018, Paul B Mahol wrote: > >> On 2/22/18, Tomas Haerdin <tjoppen@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@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
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);
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@passwd.hu> --- libavformat/mxfdec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)