Re: [FFmpeg-devel] [PATCH 2/4] avformat/mxf: get rid of samples per frame array usage
ons 2020-03-04 klockan 19:58 +0100 skrev Michael Niedermayer:
> On Tue, Mar 03, 2020 at 07:54:55PM +0100, Tomas Härdin wrote:
> > mån 2020-03-02 klockan 21:35 +0100 skrev Marton Balint:
> > > On Mon, 2 Mar 2020, Tomas Härdin wrote:
> > >
> > > > fre 2020-02-28 klockan 01:37 +0100 skrev Marton Balint:
> > > > > Signed-off-by: Marton Balint
> > > > > ---
> > > > > libavformat/mxf.c| 44
> > > > >
> > > > > libavformat/mxf.h| 6 --
> > > > > libavformat/mxfdec.c | 23 +++
> > > > > libavformat/mxfenc.c | 24 ++--
> > > > > 4 files changed, 13 insertions(+), 84 deletions(-)
> > > > > int ff_mxf_get_content_package_rate(AVRational time_base)
> > > > > {
> > > > > -int idx = av_find_nearest_q_idx(time_base, mxf_time_base);
> > > > > -AVRational diff = av_sub_q(time_base, mxf_time_base[idx]);
> > > > > -
> > > > > -diff.num = FFABS(diff.num);
> > > > > -
> > > > > -if (av_cmp_q(diff, (AVRational){1, 1000}) >= 0)
> > > > > -return -1;
> > > > > -
> > > > > -return mxf_content_package_rates[idx];
> > > > > +for (int i = 0; mxf_time_base[i].num; i++)
> > > > > +if (!av_cmp_q(time_base, mxf_time_base[i]))
> > > >
> > > > I see this gets rid of the inexact check for an exact one. Approve!
> > > >
> > > > > @@ -3307,20 +3307,17 @@ static int
> > > > > mxf_get_next_track_edit_unit(MXFContext *mxf, MXFTrack *track, int64_
> > > > > static int64_t mxf_compute_sample_count(MXFContext *mxf, AVStream
> > > > > *st,
> > > > > int64_t edit_unit)
> > > > > {
> > > > > -int i, total = 0, size = 0;
> > > > > MXFTrack *track = st->priv_data;
> > > > > AVRational time_base = av_inv_q(track->edit_rate);
> > > > > AVRational sample_rate = av_inv_q(st->time_base);
> > > > > -const MXFSamplesPerFrame *spf = NULL;
> > > > > -int64_t sample_count;
> > > > >
> > > > > // For non-audio sample_count equals current edit unit
> > > > > if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
> > > > > return edit_unit;
> > > > >
> > > > > -if ((sample_rate.num / sample_rate.den) == 48000)
> > > > > -spf = ff_mxf_get_samples_per_frame(mxf->fc, time_base);
> > > > > -if (!spf) {
> > > > > +if ((sample_rate.num / sample_rate.den) == 48000) {
> > > > > +return av_rescale_q(edit_unit, sample_rate,
> > > > > track->edit_rate);
> > > >
> > > > Should be OK, per previous rounding argument
> > > >
> > > > > }
> > > > > sc->index = INDEX_D10_AUDIO;
> > > > > sc->container_ul =
> > > > > ((MXFStreamContext*)s->streams[0]->priv_data)->container_ul;
> > > > > -sc->frame_size = 4 + 8 * spf[0].samples_per_frame[0]
> > > > > * 4;
> > > > > +sc->frame_size = 4 + 8 * av_rescale_rnd(st-
> > > > > > codecpar->sample_rate, mxf->time_base.num, mxf->time_base.den,
> > > > > AV_ROUND_UP) * 4;
> > > >
> > > > I was going to say this is only used for CBR video, but closer
> > > > inspection reveals it's used to prevent 1/1.001 rate audio packets from
> > > > making their way into CBR files. This is a bit surprising to me, since
> > > > D-10 supports NTSC (without audio?)
> > >
> > > I thought D10 can only be CBR and and it can only use a constant edit
> > > unit
> > > size, 1/1.001 audio packet size difference is handled using KLV
> > > padding. So what we compute here is a _maximum_ frame size.
> >
> > Now I remember: D-10 is indeed CBR, but at the MXF/CP level. To make it
> > CBR you must pad the content packages with fill KLVs as necessary. This
> > filling was actually removed by Baptiste a while back, and we had a set
> > of patches attempting to fix support for NTSC but sadly the ratecontrol
> > in lavc is not up to snuff to support that for files longer than about
> > an hour.
>
> Do you have a testcase for this so the ratecontrol can be fixed by fixing
> the testcase ?
See the thread "[FFmpeg-devel] [PATCH] libavformat/mxfenc: Allow more
bitrates for NTSC IMX50" in the archives. The basic problem is that it
is not possible to ask the ratecontrol system to produce packets with a
specified size. It is only possible to use bitrate, which never cleanly
divides by 30/1.001 for IMX
/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/4] avformat/mxf: get rid of samples per frame array usage
On Tue, Mar 03, 2020 at 07:54:55PM +0100, Tomas Härdin wrote:
> mån 2020-03-02 klockan 21:35 +0100 skrev Marton Balint:
> >
> > On Mon, 2 Mar 2020, Tomas Härdin wrote:
> >
> > > fre 2020-02-28 klockan 01:37 +0100 skrev Marton Balint:
> > > > Signed-off-by: Marton Balint
> > > > ---
> > > > libavformat/mxf.c| 44
> > > > libavformat/mxf.h| 6 --
> > > > libavformat/mxfdec.c | 23 +++
> > > > libavformat/mxfenc.c | 24 ++--
> > > > 4 files changed, 13 insertions(+), 84 deletions(-)
> > > > int ff_mxf_get_content_package_rate(AVRational time_base)
> > > > {
> > > > -int idx = av_find_nearest_q_idx(time_base, mxf_time_base);
> > > > -AVRational diff = av_sub_q(time_base, mxf_time_base[idx]);
> > > > -
> > > > -diff.num = FFABS(diff.num);
> > > > -
> > > > -if (av_cmp_q(diff, (AVRational){1, 1000}) >= 0)
> > > > -return -1;
> > > > -
> > > > -return mxf_content_package_rates[idx];
> > > > +for (int i = 0; mxf_time_base[i].num; i++)
> > > > +if (!av_cmp_q(time_base, mxf_time_base[i]))
> > >
> > > I see this gets rid of the inexact check for an exact one. Approve!
> > >
> > > > @@ -3307,20 +3307,17 @@ static int
> > > > mxf_get_next_track_edit_unit(MXFContext *mxf, MXFTrack *track, int64_
> > > > static int64_t mxf_compute_sample_count(MXFContext *mxf, AVStream *st,
> > > > int64_t edit_unit)
> > > > {
> > > > -int i, total = 0, size = 0;
> > > > MXFTrack *track = st->priv_data;
> > > > AVRational time_base = av_inv_q(track->edit_rate);
> > > > AVRational sample_rate = av_inv_q(st->time_base);
> > > > -const MXFSamplesPerFrame *spf = NULL;
> > > > -int64_t sample_count;
> > > >
> > > > // For non-audio sample_count equals current edit unit
> > > > if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
> > > > return edit_unit;
> > > >
> > > > -if ((sample_rate.num / sample_rate.den) == 48000)
> > > > -spf = ff_mxf_get_samples_per_frame(mxf->fc, time_base);
> > > > -if (!spf) {
> > > > +if ((sample_rate.num / sample_rate.den) == 48000) {
> > > > +return av_rescale_q(edit_unit, sample_rate, track->edit_rate);
> > >
> > > Should be OK, per previous rounding argument
> > >
> > > > }
> > > > sc->index = INDEX_D10_AUDIO;
> > > > sc->container_ul =
> > > > ((MXFStreamContext*)s->streams[0]->priv_data)->container_ul;
> > > > -sc->frame_size = 4 + 8 * spf[0].samples_per_frame[0] *
> > > > 4;
> > > > +sc->frame_size = 4 + 8 * av_rescale_rnd(st-
> > > > >codecpar->sample_rate, mxf->time_base.num, mxf->time_base.den,
> > > > AV_ROUND_UP) * 4;
> > >
> > > I was going to say this is only used for CBR video, but closer
> > > inspection reveals it's used to prevent 1/1.001 rate audio packets from
> > > making their way into CBR files. This is a bit surprising to me, since
> > > D-10 supports NTSC (without audio?)
> >
> > I thought D10 can only be CBR and and it can only use a constant edit unit
> > size, 1/1.001 audio packet size difference is handled using KLV
> > padding. So what we compute here is a _maximum_ frame size.
>
> Now I remember: D-10 is indeed CBR, but at the MXF/CP level. To make it
> CBR you must pad the content packages with fill KLVs as necessary. This
> filling was actually removed by Baptiste a while back, and we had a set
> of patches attempting to fix support for NTSC but sadly the ratecontrol
> in lavc is not up to snuff to support that for files longer than about
> an hour.
Do you have a testcase for this so the ratecontrol can be fixed by fixing
the testcase ?
Thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/4] avformat/mxf: get rid of samples per frame array usage
On Tue, 3 Mar 2020, Baptiste Coudurier wrote:
On Mar 3, 2020, at 11:37 AM, Tomas Härdin wrote:
tis 2020-03-03 klockan 11:22 -0800 skrev Baptiste Coudurier:
Hey guys,
On Mar 3, 2020, at 10:54 AM, Tomas Härdin wrote:
mån 2020-03-02 klockan 21:35 +0100 skrev Marton Balint:
On Mon, 2 Mar 2020, Tomas Härdin wrote:
fre 2020-02-28 klockan 01:37 +0100 skrev Marton Balint:
Signed-off-by: Marton Balint
---
libavformat/mxf.c| 44
libavformat/mxf.h| 6 --
libavformat/mxfdec.c | 23 +++
libavformat/mxfenc.c | 24 ++--
4 files changed, 13 insertions(+), 84 deletions(-)
int ff_mxf_get_content_package_rate(AVRational time_base)
{
-int idx = av_find_nearest_q_idx(time_base, mxf_time_base);
-AVRational diff = av_sub_q(time_base, mxf_time_base[idx]);
-
-diff.num = FFABS(diff.num);
-
-if (av_cmp_q(diff, (AVRational){1, 1000}) >= 0)
-return -1;
-
-return mxf_content_package_rates[idx];
+for (int i = 0; mxf_time_base[i].num; i++)
+if (!av_cmp_q(time_base, mxf_time_base[i]))
I see this gets rid of the inexact check for an exact one. Approve!
@@ -3307,20 +3307,17 @@ static int mxf_get_next_track_edit_unit(MXFContext
*mxf, MXFTrack *track, int64_
static int64_t mxf_compute_sample_count(MXFContext *mxf, AVStream *st,
int64_t edit_unit)
{
-int i, total = 0, size = 0;
MXFTrack *track = st->priv_data;
AVRational time_base = av_inv_q(track->edit_rate);
AVRational sample_rate = av_inv_q(st->time_base);
-const MXFSamplesPerFrame *spf = NULL;
-int64_t sample_count;
// For non-audio sample_count equals current edit unit
if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
return edit_unit;
-if ((sample_rate.num / sample_rate.den) == 48000)
-spf = ff_mxf_get_samples_per_frame(mxf->fc, time_base);
-if (!spf) {
+if ((sample_rate.num / sample_rate.den) == 48000) {
+return av_rescale_q(edit_unit, sample_rate, track->edit_rate);
Should be OK, per previous rounding argument
}
sc->index = INDEX_D10_AUDIO;
sc->container_ul =
((MXFStreamContext*)s->streams[0]->priv_data)->container_ul;
-sc->frame_size = 4 + 8 * spf[0].samples_per_frame[0] * 4;
+sc->frame_size = 4 + 8 * av_rescale_rnd(st-
codecpar->sample_rate, mxf->time_base.num, mxf->time_base.den,
AV_ROUND_UP) * 4;
I was going to say this is only used for CBR video, but closer
inspection reveals it's used to prevent 1/1.001 rate audio packets from
making their way into CBR files. This is a bit surprising to me, since
D-10 supports NTSC (without audio?)
I thought D10 can only be CBR and and it can only use a constant edit unit
size, 1/1.001 audio packet size difference is handled using KLV
padding. So what we compute here is a _maximum_ frame size.
Now I remember: D-10 is indeed CBR, but at the MXF/CP level. To make it
CBR you must pad the content packages with fill KLVs as necessary. This
filling was actually removed by Baptiste a while back, and we had a set
of patches attempting to fix support for NTSC but sadly the ratecontrol
in lavc is not up to snuff to support that for files longer than about
an hour. This also means the video essence needs to be coded at a
bitrate slightly lower than the D-10 rate, say 49.9 Mbps for 50 Mbps D-
10 (+-audio).
IIRC, and correct me if I’m wrong, the VBV delay in D-10 is actually fixed,
so the essence needs to be CBR at the essence (mpeg-2) level.
Anything else is just broken IMHO.
For practical D-10 decoders probably, but I think the spec is a bit
more lax. You only need to set up the DeltaEntry array in the
IndexTableSegment correctly
I believe at some point there was code for padding the MPEG-2 essence
with zeroes to make CBR, but that obviously doesn't work with audio due
to 1601.6 samples per EditUnit
Yeah, the pattern of audio samples is very strict in MXF, and every 5 frames,
Audio and video durations match perfectly.
Yep, which luckily this patchset does. For audio there definitely needs
to be KLV fill since the size of the audio packets will vary
Wait, is it what it does ? The pattern is very strict and ordered.
For 3/1001 and 6/1001, it seems to me that it would not be correct.
The patchset replaces the hard coded sample tables with mathematically
computed values, because
samples_in_nth_frame = av_rescale_q(n+1, {48000,1}, frame_rate) -
av_rescale_q(n, {48000,1}, frame_rate);
And this is also true for 3/1001 and 6/1001 frame rates.
Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/4] avformat/mxf: get rid of samples per frame array usage
> On Mar 3, 2020, at 11:37 AM, Tomas Härdin wrote:
>
> tis 2020-03-03 klockan 11:22 -0800 skrev Baptiste Coudurier:
>> Hey guys,
>>
>>
>>> On Mar 3, 2020, at 10:54 AM, Tomas Härdin wrote:
>>>
>>> mån 2020-03-02 klockan 21:35 +0100 skrev Marton Balint:
On Mon, 2 Mar 2020, Tomas Härdin wrote:
> fre 2020-02-28 klockan 01:37 +0100 skrev Marton Balint:
>> Signed-off-by: Marton Balint
>> ---
>> libavformat/mxf.c| 44
>> libavformat/mxf.h| 6 --
>> libavformat/mxfdec.c | 23 +++
>> libavformat/mxfenc.c | 24 ++--
>> 4 files changed, 13 insertions(+), 84 deletions(-)
>> int ff_mxf_get_content_package_rate(AVRational time_base)
>> {
>> -int idx = av_find_nearest_q_idx(time_base, mxf_time_base);
>> -AVRational diff = av_sub_q(time_base, mxf_time_base[idx]);
>> -
>> -diff.num = FFABS(diff.num);
>> -
>> -if (av_cmp_q(diff, (AVRational){1, 1000}) >= 0)
>> -return -1;
>> -
>> -return mxf_content_package_rates[idx];
>> +for (int i = 0; mxf_time_base[i].num; i++)
>> +if (!av_cmp_q(time_base, mxf_time_base[i]))
>
> I see this gets rid of the inexact check for an exact one. Approve!
>
>> @@ -3307,20 +3307,17 @@ static int
>> mxf_get_next_track_edit_unit(MXFContext *mxf, MXFTrack *track, int64_
>> static int64_t mxf_compute_sample_count(MXFContext *mxf, AVStream *st,
>>int64_t edit_unit)
>> {
>> -int i, total = 0, size = 0;
>>MXFTrack *track = st->priv_data;
>>AVRational time_base = av_inv_q(track->edit_rate);
>>AVRational sample_rate = av_inv_q(st->time_base);
>> -const MXFSamplesPerFrame *spf = NULL;
>> -int64_t sample_count;
>>
>>// For non-audio sample_count equals current edit unit
>>if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
>>return edit_unit;
>>
>> -if ((sample_rate.num / sample_rate.den) == 48000)
>> -spf = ff_mxf_get_samples_per_frame(mxf->fc, time_base);
>> -if (!spf) {
>> +if ((sample_rate.num / sample_rate.den) == 48000) {
>> +return av_rescale_q(edit_unit, sample_rate, track->edit_rate);
>
> Should be OK, per previous rounding argument
>
>>}
>>sc->index = INDEX_D10_AUDIO;
>>sc->container_ul =
>> ((MXFStreamContext*)s->streams[0]->priv_data)->container_ul;
>> -sc->frame_size = 4 + 8 * spf[0].samples_per_frame[0] *
>> 4;
>> +sc->frame_size = 4 + 8 * av_rescale_rnd(st-
>>> codecpar->sample_rate, mxf->time_base.num, mxf->time_base.den,
>> AV_ROUND_UP) * 4;
>
> I was going to say this is only used for CBR video, but closer
> inspection reveals it's used to prevent 1/1.001 rate audio packets from
> making their way into CBR files. This is a bit surprising to me, since
> D-10 supports NTSC (without audio?)
I thought D10 can only be CBR and and it can only use a constant edit unit
size, 1/1.001 audio packet size difference is handled using KLV
padding. So what we compute here is a _maximum_ frame size.
>>>
>>> Now I remember: D-10 is indeed CBR, but at the MXF/CP level. To make it
>>> CBR you must pad the content packages with fill KLVs as necessary. This
>>> filling was actually removed by Baptiste a while back, and we had a set
>>> of patches attempting to fix support for NTSC but sadly the ratecontrol
>>> in lavc is not up to snuff to support that for files longer than about
>>> an hour. This also means the video essence needs to be coded at a
>>> bitrate slightly lower than the D-10 rate, say 49.9 Mbps for 50 Mbps D-
>>> 10 (+-audio).
>>
>> IIRC, and correct me if I’m wrong, the VBV delay in D-10 is actually fixed,
>> so the essence needs to be CBR at the essence (mpeg-2) level.
>> Anything else is just broken IMHO.
>
> For practical D-10 decoders probably, but I think the spec is a bit
> more lax. You only need to set up the DeltaEntry array in the
> IndexTableSegment correctly
>
>>> I believe at some point there was code for padding the MPEG-2 essence
>>> with zeroes to make CBR, but that obviously doesn't work with audio due
>>> to 1601.6 samples per EditUnit
>>
>> Yeah, the pattern of audio samples is very strict in MXF, and every 5
>> frames,
>> Audio and video durations match perfectly.
>
> Yep, which luckily this patchset does. For audio there definitely needs
> to be KLV fill since the size of the audio packets will vary
Wait, is it what it does ? The pattern is very strict and ordered.
For 3/1001 and 6/1001, it seems to me that it would not be correct.
—
Baptiste
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
Re: [FFmpeg-devel] [PATCH 2/4] avformat/mxf: get rid of samples per frame array usage
tis 2020-03-03 klockan 11:22 -0800 skrev Baptiste Coudurier:
> Hey guys,
>
>
> > On Mar 3, 2020, at 10:54 AM, Tomas Härdin wrote:
> >
> > mån 2020-03-02 klockan 21:35 +0100 skrev Marton Balint:
> > > On Mon, 2 Mar 2020, Tomas Härdin wrote:
> > >
> > > > fre 2020-02-28 klockan 01:37 +0100 skrev Marton Balint:
> > > > > Signed-off-by: Marton Balint
> > > > > ---
> > > > > libavformat/mxf.c| 44
> > > > > libavformat/mxf.h| 6 --
> > > > > libavformat/mxfdec.c | 23 +++
> > > > > libavformat/mxfenc.c | 24 ++--
> > > > > 4 files changed, 13 insertions(+), 84 deletions(-)
> > > > > int ff_mxf_get_content_package_rate(AVRational time_base)
> > > > > {
> > > > > -int idx = av_find_nearest_q_idx(time_base, mxf_time_base);
> > > > > -AVRational diff = av_sub_q(time_base, mxf_time_base[idx]);
> > > > > -
> > > > > -diff.num = FFABS(diff.num);
> > > > > -
> > > > > -if (av_cmp_q(diff, (AVRational){1, 1000}) >= 0)
> > > > > -return -1;
> > > > > -
> > > > > -return mxf_content_package_rates[idx];
> > > > > +for (int i = 0; mxf_time_base[i].num; i++)
> > > > > +if (!av_cmp_q(time_base, mxf_time_base[i]))
> > > >
> > > > I see this gets rid of the inexact check for an exact one. Approve!
> > > >
> > > > > @@ -3307,20 +3307,17 @@ static int
> > > > > mxf_get_next_track_edit_unit(MXFContext *mxf, MXFTrack *track, int64_
> > > > > static int64_t mxf_compute_sample_count(MXFContext *mxf, AVStream *st,
> > > > > int64_t edit_unit)
> > > > > {
> > > > > -int i, total = 0, size = 0;
> > > > > MXFTrack *track = st->priv_data;
> > > > > AVRational time_base = av_inv_q(track->edit_rate);
> > > > > AVRational sample_rate = av_inv_q(st->time_base);
> > > > > -const MXFSamplesPerFrame *spf = NULL;
> > > > > -int64_t sample_count;
> > > > >
> > > > > // For non-audio sample_count equals current edit unit
> > > > > if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
> > > > > return edit_unit;
> > > > >
> > > > > -if ((sample_rate.num / sample_rate.den) == 48000)
> > > > > -spf = ff_mxf_get_samples_per_frame(mxf->fc, time_base);
> > > > > -if (!spf) {
> > > > > +if ((sample_rate.num / sample_rate.den) == 48000) {
> > > > > +return av_rescale_q(edit_unit, sample_rate,
> > > > > track->edit_rate);
> > > >
> > > > Should be OK, per previous rounding argument
> > > >
> > > > > }
> > > > > sc->index = INDEX_D10_AUDIO;
> > > > > sc->container_ul =
> > > > > ((MXFStreamContext*)s->streams[0]->priv_data)->container_ul;
> > > > > -sc->frame_size = 4 + 8 * spf[0].samples_per_frame[0]
> > > > > * 4;
> > > > > +sc->frame_size = 4 + 8 * av_rescale_rnd(st-
> > > > > > codecpar->sample_rate, mxf->time_base.num, mxf->time_base.den,
> > > > > AV_ROUND_UP) * 4;
> > > >
> > > > I was going to say this is only used for CBR video, but closer
> > > > inspection reveals it's used to prevent 1/1.001 rate audio packets from
> > > > making their way into CBR files. This is a bit surprising to me, since
> > > > D-10 supports NTSC (without audio?)
> > >
> > > I thought D10 can only be CBR and and it can only use a constant edit
> > > unit
> > > size, 1/1.001 audio packet size difference is handled using KLV
> > > padding. So what we compute here is a _maximum_ frame size.
> >
> > Now I remember: D-10 is indeed CBR, but at the MXF/CP level. To make it
> > CBR you must pad the content packages with fill KLVs as necessary. This
> > filling was actually removed by Baptiste a while back, and we had a set
> > of patches attempting to fix support for NTSC but sadly the ratecontrol
> > in lavc is not up to snuff to support that for files longer than about
> > an hour. This also means the video essence needs to be coded at a
> > bitrate slightly lower than the D-10 rate, say 49.9 Mbps for 50 Mbps D-
> > 10 (+-audio).
>
> IIRC, and correct me if I’m wrong, the VBV delay in D-10 is actually fixed,
> so the essence needs to be CBR at the essence (mpeg-2) level.
> Anything else is just broken IMHO.
For practical D-10 decoders probably, but I think the spec is a bit
more lax. You only need to set up the DeltaEntry array in the
IndexTableSegment correctly
> > I believe at some point there was code for padding the MPEG-2 essence
> > with zeroes to make CBR, but that obviously doesn't work with audio due
> > to 1601.6 samples per EditUnit
>
> Yeah, the pattern of audio samples is very strict in MXF, and every 5 frames,
> Audio and video durations match perfectly.
Yep, which luckily this patchset does. For audio there definitely needs
to be KLV fill since the size of the audio packets will vary
/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
Re: [FFmpeg-devel] [PATCH 2/4] avformat/mxf: get rid of samples per frame array usage
Hey guys,
> On Mar 3, 2020, at 10:54 AM, Tomas Härdin wrote:
>
> mån 2020-03-02 klockan 21:35 +0100 skrev Marton Balint:
>>
>> On Mon, 2 Mar 2020, Tomas Härdin wrote:
>>
>>> fre 2020-02-28 klockan 01:37 +0100 skrev Marton Balint:
Signed-off-by: Marton Balint
---
libavformat/mxf.c| 44
libavformat/mxf.h| 6 --
libavformat/mxfdec.c | 23 +++
libavformat/mxfenc.c | 24 ++--
4 files changed, 13 insertions(+), 84 deletions(-)
int ff_mxf_get_content_package_rate(AVRational time_base)
{
-int idx = av_find_nearest_q_idx(time_base, mxf_time_base);
-AVRational diff = av_sub_q(time_base, mxf_time_base[idx]);
-
-diff.num = FFABS(diff.num);
-
-if (av_cmp_q(diff, (AVRational){1, 1000}) >= 0)
-return -1;
-
-return mxf_content_package_rates[idx];
+for (int i = 0; mxf_time_base[i].num; i++)
+if (!av_cmp_q(time_base, mxf_time_base[i]))
>>>
>>> I see this gets rid of the inexact check for an exact one. Approve!
>>>
@@ -3307,20 +3307,17 @@ static int mxf_get_next_track_edit_unit(MXFContext
*mxf, MXFTrack *track, int64_
static int64_t mxf_compute_sample_count(MXFContext *mxf, AVStream *st,
int64_t edit_unit)
{
-int i, total = 0, size = 0;
MXFTrack *track = st->priv_data;
AVRational time_base = av_inv_q(track->edit_rate);
AVRational sample_rate = av_inv_q(st->time_base);
-const MXFSamplesPerFrame *spf = NULL;
-int64_t sample_count;
// For non-audio sample_count equals current edit unit
if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
return edit_unit;
-if ((sample_rate.num / sample_rate.den) == 48000)
-spf = ff_mxf_get_samples_per_frame(mxf->fc, time_base);
-if (!spf) {
+if ((sample_rate.num / sample_rate.den) == 48000) {
+return av_rescale_q(edit_unit, sample_rate, track->edit_rate);
>>>
>>> Should be OK, per previous rounding argument
>>>
}
sc->index = INDEX_D10_AUDIO;
sc->container_ul =
((MXFStreamContext*)s->streams[0]->priv_data)->container_ul;
-sc->frame_size = 4 + 8 * spf[0].samples_per_frame[0] * 4;
+sc->frame_size = 4 + 8 * av_rescale_rnd(st-
> codecpar->sample_rate, mxf->time_base.num, mxf->time_base.den,
AV_ROUND_UP) * 4;
>>>
>>> I was going to say this is only used for CBR video, but closer
>>> inspection reveals it's used to prevent 1/1.001 rate audio packets from
>>> making their way into CBR files. This is a bit surprising to me, since
>>> D-10 supports NTSC (without audio?)
>>
>> I thought D10 can only be CBR and and it can only use a constant edit unit
>> size, 1/1.001 audio packet size difference is handled using KLV
>> padding. So what we compute here is a _maximum_ frame size.
>
> Now I remember: D-10 is indeed CBR, but at the MXF/CP level. To make it
> CBR you must pad the content packages with fill KLVs as necessary. This
> filling was actually removed by Baptiste a while back, and we had a set
> of patches attempting to fix support for NTSC but sadly the ratecontrol
> in lavc is not up to snuff to support that for files longer than about
> an hour. This also means the video essence needs to be coded at a
> bitrate slightly lower than the D-10 rate, say 49.9 Mbps for 50 Mbps D-
> 10 (+-audio).
IIRC, and correct me if I’m wrong, the VBV delay in D-10 is actually fixed,
so the essence needs to be CBR at the essence (mpeg-2) level.
Anything else is just broken IMHO.
> I believe at some point there was code for padding the MPEG-2 essence
> with zeroes to make CBR, but that obviously doesn't work with audio due
> to 1601.6 samples per EditUnit
Yeah, the pattern of audio samples is very strict in MXF, and every 5 frames,
Audio and video durations match perfectly.
—
Baptiste
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/4] avformat/mxf: get rid of samples per frame array usage
mån 2020-03-02 klockan 21:35 +0100 skrev Marton Balint:
>
> On Mon, 2 Mar 2020, Tomas Härdin wrote:
>
> > fre 2020-02-28 klockan 01:37 +0100 skrev Marton Balint:
> > > Signed-off-by: Marton Balint
> > > ---
> > > libavformat/mxf.c| 44
> > > libavformat/mxf.h| 6 --
> > > libavformat/mxfdec.c | 23 +++
> > > libavformat/mxfenc.c | 24 ++--
> > > 4 files changed, 13 insertions(+), 84 deletions(-)
> > > int ff_mxf_get_content_package_rate(AVRational time_base)
> > > {
> > > -int idx = av_find_nearest_q_idx(time_base, mxf_time_base);
> > > -AVRational diff = av_sub_q(time_base, mxf_time_base[idx]);
> > > -
> > > -diff.num = FFABS(diff.num);
> > > -
> > > -if (av_cmp_q(diff, (AVRational){1, 1000}) >= 0)
> > > -return -1;
> > > -
> > > -return mxf_content_package_rates[idx];
> > > +for (int i = 0; mxf_time_base[i].num; i++)
> > > +if (!av_cmp_q(time_base, mxf_time_base[i]))
> >
> > I see this gets rid of the inexact check for an exact one. Approve!
> >
> > > @@ -3307,20 +3307,17 @@ static int
> > > mxf_get_next_track_edit_unit(MXFContext *mxf, MXFTrack *track, int64_
> > > static int64_t mxf_compute_sample_count(MXFContext *mxf, AVStream *st,
> > > int64_t edit_unit)
> > > {
> > > -int i, total = 0, size = 0;
> > > MXFTrack *track = st->priv_data;
> > > AVRational time_base = av_inv_q(track->edit_rate);
> > > AVRational sample_rate = av_inv_q(st->time_base);
> > > -const MXFSamplesPerFrame *spf = NULL;
> > > -int64_t sample_count;
> > >
> > > // For non-audio sample_count equals current edit unit
> > > if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
> > > return edit_unit;
> > >
> > > -if ((sample_rate.num / sample_rate.den) == 48000)
> > > -spf = ff_mxf_get_samples_per_frame(mxf->fc, time_base);
> > > -if (!spf) {
> > > +if ((sample_rate.num / sample_rate.den) == 48000) {
> > > +return av_rescale_q(edit_unit, sample_rate, track->edit_rate);
> >
> > Should be OK, per previous rounding argument
> >
> > > }
> > > sc->index = INDEX_D10_AUDIO;
> > > sc->container_ul =
> > > ((MXFStreamContext*)s->streams[0]->priv_data)->container_ul;
> > > -sc->frame_size = 4 + 8 * spf[0].samples_per_frame[0] * 4;
> > > +sc->frame_size = 4 + 8 * av_rescale_rnd(st-
> > > >codecpar->sample_rate, mxf->time_base.num, mxf->time_base.den,
> > > AV_ROUND_UP) * 4;
> >
> > I was going to say this is only used for CBR video, but closer
> > inspection reveals it's used to prevent 1/1.001 rate audio packets from
> > making their way into CBR files. This is a bit surprising to me, since
> > D-10 supports NTSC (without audio?)
>
> I thought D10 can only be CBR and and it can only use a constant edit unit
> size, 1/1.001 audio packet size difference is handled using KLV
> padding. So what we compute here is a _maximum_ frame size.
Now I remember: D-10 is indeed CBR, but at the MXF/CP level. To make it
CBR you must pad the content packages with fill KLVs as necessary. This
filling was actually removed by Baptiste a while back, and we had a set
of patches attempting to fix support for NTSC but sadly the ratecontrol
in lavc is not up to snuff to support that for files longer than about
an hour. This also means the video essence needs to be coded at a
bitrate slightly lower than the D-10 rate, say 49.9 Mbps for 50 Mbps D-
10 (+-audio).
I believe at some point there was code for padding the MPEG-2 essence
with zeroes to make CBR, but that obviously doesn't work with audio due
to 1601.6 samples per EditUnit
/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/4] avformat/mxf: get rid of samples per frame array usage
On Mon, 2 Mar 2020, Tomas Härdin wrote:
fre 2020-02-28 klockan 01:37 +0100 skrev Marton Balint:
Signed-off-by: Marton Balint
---
libavformat/mxf.c| 44
libavformat/mxf.h| 6 --
libavformat/mxfdec.c | 23 +++
libavformat/mxfenc.c | 24 ++--
4 files changed, 13 insertions(+), 84 deletions(-)
int ff_mxf_get_content_package_rate(AVRational time_base)
{
-int idx = av_find_nearest_q_idx(time_base, mxf_time_base);
-AVRational diff = av_sub_q(time_base, mxf_time_base[idx]);
-
-diff.num = FFABS(diff.num);
-
-if (av_cmp_q(diff, (AVRational){1, 1000}) >= 0)
-return -1;
-
-return mxf_content_package_rates[idx];
+for (int i = 0; mxf_time_base[i].num; i++)
+if (!av_cmp_q(time_base, mxf_time_base[i]))
I see this gets rid of the inexact check for an exact one. Approve!
@@ -3307,20 +3307,17 @@ static int mxf_get_next_track_edit_unit(MXFContext
*mxf, MXFTrack *track, int64_
static int64_t mxf_compute_sample_count(MXFContext *mxf, AVStream *st,
int64_t edit_unit)
{
-int i, total = 0, size = 0;
MXFTrack *track = st->priv_data;
AVRational time_base = av_inv_q(track->edit_rate);
AVRational sample_rate = av_inv_q(st->time_base);
-const MXFSamplesPerFrame *spf = NULL;
-int64_t sample_count;
// For non-audio sample_count equals current edit unit
if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
return edit_unit;
-if ((sample_rate.num / sample_rate.den) == 48000)
-spf = ff_mxf_get_samples_per_frame(mxf->fc, time_base);
-if (!spf) {
+if ((sample_rate.num / sample_rate.den) == 48000) {
+return av_rescale_q(edit_unit, sample_rate, track->edit_rate);
Should be OK, per previous rounding argument
}
sc->index = INDEX_D10_AUDIO;
sc->container_ul =
((MXFStreamContext*)s->streams[0]->priv_data)->container_ul;
-sc->frame_size = 4 + 8 * spf[0].samples_per_frame[0] * 4;
+sc->frame_size = 4 + 8 * av_rescale_rnd(st->codecpar->sample_rate,
mxf->time_base.num, mxf->time_base.den, AV_ROUND_UP) * 4;
I was going to say this is only used for CBR video, but closer
inspection reveals it's used to prevent 1/1.001 rate audio packets from
making their way into CBR files. This is a bit surprising to me, since
D-10 supports NTSC (without audio?)
I thought D10 can only be CBR and and it can only use a constant edit unit
size, 1/1.001 audio packet size difference is handled using KLV
padding. So what we compute here is a _maximum_ frame size.
Regards,
Marton
sc->index = INDEX_WAV;
} else {
mxf->slice_count = 1;
-sc->frame_size = (st->codecpar->channels *
spf[0].samples_per_frame[0] *
-
av_get_bits_per_sample(st->codecpar->codec_id)) / 8;
+sc->frame_size = st->codecpar->channels *
+ av_rescale_rnd(st->codecpar->sample_rate,
mxf->time_base.num, mxf->time_base.den, AV_ROUND_UP) *
+
av_get_bits_per_sample(st->codecpar->codec_id) / 8;
Looks similarly OK
/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/4] avformat/mxf: get rid of samples per frame array usage
fre 2020-02-28 klockan 01:37 +0100 skrev Marton Balint:
> Signed-off-by: Marton Balint
> ---
> libavformat/mxf.c| 44
> libavformat/mxf.h| 6 --
> libavformat/mxfdec.c | 23 +++
> libavformat/mxfenc.c | 24 ++--
> 4 files changed, 13 insertions(+), 84 deletions(-)
> int ff_mxf_get_content_package_rate(AVRational time_base)
> {
> -int idx = av_find_nearest_q_idx(time_base, mxf_time_base);
> -AVRational diff = av_sub_q(time_base, mxf_time_base[idx]);
> -
> -diff.num = FFABS(diff.num);
> -
> -if (av_cmp_q(diff, (AVRational){1, 1000}) >= 0)
> -return -1;
> -
> -return mxf_content_package_rates[idx];
> +for (int i = 0; mxf_time_base[i].num; i++)
> +if (!av_cmp_q(time_base, mxf_time_base[i]))
I see this gets rid of the inexact check for an exact one. Approve!
> @@ -3307,20 +3307,17 @@ static int mxf_get_next_track_edit_unit(MXFContext
> *mxf, MXFTrack *track, int64_
> static int64_t mxf_compute_sample_count(MXFContext *mxf, AVStream *st,
> int64_t edit_unit)
> {
> -int i, total = 0, size = 0;
> MXFTrack *track = st->priv_data;
> AVRational time_base = av_inv_q(track->edit_rate);
> AVRational sample_rate = av_inv_q(st->time_base);
> -const MXFSamplesPerFrame *spf = NULL;
> -int64_t sample_count;
>
> // For non-audio sample_count equals current edit unit
> if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
> return edit_unit;
>
> -if ((sample_rate.num / sample_rate.den) == 48000)
> -spf = ff_mxf_get_samples_per_frame(mxf->fc, time_base);
> -if (!spf) {
> +if ((sample_rate.num / sample_rate.den) == 48000) {
> +return av_rescale_q(edit_unit, sample_rate, track->edit_rate);
Should be OK, per previous rounding argument
> }
> sc->index = INDEX_D10_AUDIO;
> sc->container_ul =
> ((MXFStreamContext*)s->streams[0]->priv_data)->container_ul;
> -sc->frame_size = 4 + 8 * spf[0].samples_per_frame[0] * 4;
> +sc->frame_size = 4 + 8 *
> av_rescale_rnd(st->codecpar->sample_rate, mxf->time_base.num,
> mxf->time_base.den, AV_ROUND_UP) * 4;
I was going to say this is only used for CBR video, but closer
inspection reveals it's used to prevent 1/1.001 rate audio packets from
making their way into CBR files. This is a bit surprising to me, since
D-10 supports NTSC (without audio?)
> sc->index = INDEX_WAV;
> } else {
> mxf->slice_count = 1;
> -sc->frame_size = (st->codecpar->channels *
> spf[0].samples_per_frame[0] *
> -
> av_get_bits_per_sample(st->codecpar->codec_id)) / 8;
> +sc->frame_size = st->codecpar->channels *
> + av_rescale_rnd(st->codecpar->sample_rate,
> mxf->time_base.num, mxf->time_base.den, AV_ROUND_UP) *
> +
> av_get_bits_per_sample(st->codecpar->codec_id) / 8;
Looks similarly OK
/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 2/4] avformat/mxf: get rid of samples per frame array usage
Signed-off-by: Marton Balint
---
libavformat/mxf.c| 44
libavformat/mxf.h| 6 --
libavformat/mxfdec.c | 23 +++
libavformat/mxfenc.c | 24 ++--
4 files changed, 13 insertions(+), 84 deletions(-)
diff --git a/libavformat/mxf.c b/libavformat/mxf.c
index 451cbcfb2c..10ccd770e3 100644
--- a/libavformat/mxf.c
+++ b/libavformat/mxf.c
@@ -131,16 +131,6 @@ int ff_mxf_decode_pixel_layout(const char
pixel_layout[16], enum AVPixelFormat *
return -1;
}
-static const MXFSamplesPerFrame mxf_spf[] = {
-{ { 1001, 24000 }, { 2002, 0,0,0,0,0 } }, // FILM 23.976
-{ { 1, 24},{ 2000, 0,0,0,0,0 } }, // FILM 24
-{ { 1001, 3 }, { 1602, 1601, 1602, 1601, 1602, 0 } }, // NTSC 29.97
-{ { 1001, 6 }, { 801, 801, 800, 801, 801, 0 } }, // NTSC 59.94
-{ { 1, 25 }, { 1920, 0,0,0,0,0 } }, // PAL 25
-{ { 1, 50 }, { 960, 0,0,0,0,0 } }, // PAL 50
-{ { 1, 60 }, { 800, 0,0,0,0,0 } },
-};
-
static const AVRational mxf_time_base[] = {
{ 1001, 24000 },
{ 1, 24},
@@ -152,40 +142,14 @@ static const AVRational mxf_time_base[] = {
{ 0, 0}
};
-const MXFSamplesPerFrame *ff_mxf_get_samples_per_frame(AVFormatContext *s,
- AVRational time_base)
-{
-int idx = av_find_nearest_q_idx(time_base, mxf_time_base);
-AVRational diff = av_sub_q(time_base, mxf_time_base[idx]);
-
-diff.num = FFABS(diff.num);
-
-if (av_cmp_q(diff, (AVRational){1, 1000}) >= 0)
-return NULL;
-
-if (av_cmp_q(time_base, mxf_time_base[idx]))
-av_log(s, AV_LOG_WARNING,
- "%d/%d input time base matched %d/%d container time base\n",
- time_base.num, time_base.den,
- mxf_spf[idx].time_base.num,
- mxf_spf[idx].time_base.den);
-
-return _spf[idx];
-}
-
static const int mxf_content_package_rates[] = {
3, 2, 7, 13, 4, 10, 12,
};
int ff_mxf_get_content_package_rate(AVRational time_base)
{
-int idx = av_find_nearest_q_idx(time_base, mxf_time_base);
-AVRational diff = av_sub_q(time_base, mxf_time_base[idx]);
-
-diff.num = FFABS(diff.num);
-
-if (av_cmp_q(diff, (AVRational){1, 1000}) >= 0)
-return -1;
-
-return mxf_content_package_rates[idx];
+for (int i = 0; mxf_time_base[i].num; i++)
+if (!av_cmp_q(time_base, mxf_time_base[i]))
+return mxf_content_package_rates[i];
+return 0;
}
diff --git a/libavformat/mxf.h b/libavformat/mxf.h
index f32124f772..2669269830 100644
--- a/libavformat/mxf.h
+++ b/libavformat/mxf.h
@@ -82,18 +82,12 @@ typedef struct MXFCodecUL {
MXFWrappingIndicatorType wrapping_indicator_type;
} MXFCodecUL;
-typedef struct {
-struct AVRational time_base;
-int samples_per_frame[6];
-} MXFSamplesPerFrame;
-
extern const MXFCodecUL ff_mxf_data_definition_uls[];
extern const MXFCodecUL ff_mxf_codec_uls[];
extern const MXFCodecUL ff_mxf_pixel_format_uls[];
extern const MXFCodecUL ff_mxf_codec_tag_uls[];
int ff_mxf_decode_pixel_layout(const char pixel_layout[16], enum AVPixelFormat
*pix_fmt);
-const MXFSamplesPerFrame *ff_mxf_get_samples_per_frame(AVFormatContext *s,
AVRational time_base);
int ff_mxf_get_content_package_rate(AVRational time_base);
diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 9a48e2d2d1..9113e2a09c 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -3307,20 +3307,17 @@ static int mxf_get_next_track_edit_unit(MXFContext
*mxf, MXFTrack *track, int64_
static int64_t mxf_compute_sample_count(MXFContext *mxf, AVStream *st,
int64_t edit_unit)
{
-int i, total = 0, size = 0;
MXFTrack *track = st->priv_data;
AVRational time_base = av_inv_q(track->edit_rate);
AVRational sample_rate = av_inv_q(st->time_base);
-const MXFSamplesPerFrame *spf = NULL;
-int64_t sample_count;
// For non-audio sample_count equals current edit unit
if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
return edit_unit;
-if ((sample_rate.num / sample_rate.den) == 48000)
-spf = ff_mxf_get_samples_per_frame(mxf->fc, time_base);
-if (!spf) {
+if ((sample_rate.num / sample_rate.den) == 48000) {
+return av_rescale_q(edit_unit, sample_rate, track->edit_rate);
+} else {
int remainder = (sample_rate.num * time_base.num) %
(time_base.den * sample_rate.den);
if (remainder)
@@ -3331,20 +3328,6 @@ static int64_t mxf_compute_sample_count(MXFContext *mxf,
AVStream *st,
sample_rate.num, sample_rate.den);
return av_rescale_q(edit_unit, sample_rate, track->edit_rate);
}
-
-while (spf->samples_per_frame[size]) {
-total += spf->samples_per_frame[size];
-size++;
-}
