mbox series

[FFmpeg-devel,v2,0/5] avformat/movenc: btrt box support

Message ID 20200921172948.32440-1-jeebjp@gmail.com
Headers show
Series avformat/movenc: btrt box support
Related show

Message

Jan Ekström Sept. 21, 2020, 5:29 p.m. UTC
Version 2:
- Only writes the btrt box if at least value is nonzero.
- Other smaller changes based on Martin's comments. 

Various media ingest servers read the incoming stream's advertised bit
rate from this box.

As it is only defined for timed metadata tracks in QTFF, limit it to
just MODE_MP4 (ISOBMFF) for now.

Unifies the MPEG-4 bit rate value calculation, and attempts to utilize
it wherever applicable.

Jan Ekström (5):
  avformat/movenc: split MPEG-4 bit rate value calculation
  avformat/movenc: utilize bit rate helper function in ISML writing
  avformat/movenc: implement writing of the btrt box
  avformat/movenc: use more fall-back values for average bit rate fields
  avformat/movenc: simplify ISML manifest bit rate logic

 libavformat/movenc.c               | 112 +++++++++++++++++++++++------
 tests/fate/mov.mak                 |   2 +-
 tests/ref/fate/copy-trac3074       |   4 +-
 tests/ref/fate/movenc              |  12 ++--
 tests/ref/lavf-fate/av1.mp4        |   4 +-
 tests/ref/lavf-fate/h264.mp4       |   4 +-
 tests/ref/lavf/ismv                |   6 +-
 tests/ref/lavf/mp4                 |  12 ++--
 tests/ref/vsynth/vsynth1-mpeg4     |   4 +-
 tests/ref/vsynth/vsynth2-mpeg4     |   4 +-
 tests/ref/vsynth/vsynth3-mpeg4     |   4 +-
 tests/ref/vsynth/vsynth_lena-mpeg4 |   4 +-
 12 files changed, 119 insertions(+), 53 deletions(-)

Comments

Martin Storsjö Sept. 22, 2020, 9:42 a.m. UTC | #1
On Mon, 21 Sep 2020, Jan Ekström wrote:

> This is utilized by various media ingests to figure out the bit
> rate of the content you are pushing towards it, so write it by
> default for video, audio and subtitle tracks. It is only mentioned
> for timed metadata sample descriptions in QTFF, so limit it only to
> ISOBMFF (MODE_MP4).
>
> Updates the FATE tests which have their results changed due to the
> 20 extra bytes being written per track.
> ---

Maybe mention in the commit message, that the box is only written if 
there's any sensible information to write into it?

Other than that, the updated patchset looks good to me - thanks!

// Martin
Jan Ekström Sept. 22, 2020, 9:58 a.m. UTC | #2
On Tue, Sep 22, 2020 at 12:42 PM Martin Storsjö <martin@martin.st> wrote:
>
> On Mon, 21 Sep 2020, Jan Ekström wrote:
>
> > This is utilized by various media ingests to figure out the bit
> > rate of the content you are pushing towards it, so write it by
> > default for video, audio and subtitle tracks. It is only mentioned
> > for timed metadata sample descriptions in QTFF, so limit it only to
> > ISOBMFF (MODE_MP4).
> >
> > Updates the FATE tests which have their results changed due to the
> > 20 extra bytes being written per track.
> > ---
>
> Maybe mention in the commit message, that the box is only written if
> there's any sensible information to write into it?
>
> Other than that, the updated patchset looks good to me - thanks!

Ah yes, forgot to update the commit message in this specific case :)

Thanks for taking a look, I will update the commit message, re-run
FATE (since patchwork seems to have missed various patches in the
series - I'm never sure how I should post updated series with
send-email) and push it in the evening after $dayjob then unless there
are any other comments.

Jan
Jan Ekström Sept. 22, 2020, 3:35 p.m. UTC | #3
On Tue, Sep 22, 2020 at 12:58 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Tue, Sep 22, 2020 at 12:42 PM Martin Storsjö <martin@martin.st> wrote:
> >
> > On Mon, 21 Sep 2020, Jan Ekström wrote:
> >
> > > This is utilized by various media ingests to figure out the bit
> > > rate of the content you are pushing towards it, so write it by
> > > default for video, audio and subtitle tracks. It is only mentioned
> > > for timed metadata sample descriptions in QTFF, so limit it only to
> > > ISOBMFF (MODE_MP4).
> > >
> > > Updates the FATE tests which have their results changed due to the
> > > 20 extra bytes being written per track.
> > > ---
> >
> > Maybe mention in the commit message, that the box is only written if
> > there's any sensible information to write into it?
> >
> > Other than that, the updated patchset looks good to me - thanks!
>
> Ah yes, forgot to update the commit message in this specific case :)
>
> Thanks for taking a look, I will update the commit message, re-run
> FATE (since patchwork seems to have missed various patches in the
> series - I'm never sure how I should post updated series with
> send-email) and push it in the evening after $dayjob then unless there
> are any other comments.
>
> Jan

Reworded, checked that FATE passes and applied the set.

Jan