Re: [FFmpeg-devel] [PATCH v1 5/8] avformat/mov_muxer: Extended MOV muxer to handle APV video content

[Dawid Kozinski/Multimedia (PLT) /SRPOL/Staff Engineer/Samsung Electronics]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of Mark
> Thompson
> Sent: czwartek, 24 kwietnia 2025 21:02
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v1 5/8] avformat/mov_muxer: Extended
> MOV muxer to handle APV video content
> 
> On 24/04/2025 13:08, Dawid Kozinski/Multimedia (PLT) /SRPOL/Staff
> Engineer/Samsung Electronics wrote:
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of
> >> Mark Thompson
> >> Sent: środa, 23 kwietnia 2025 23:08
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v1 5/8] avformat/mov_muxer:
> >> Extended MOV muxer to handle APV video content
> >>
> >> On 23/04/2025 15:13, Dawid Kozinski wrote:
> >>> - Changes in mov_write_video_tag function to handle APV elementary
> >>> stream
> >>> - Provided structure APVDecoderConfigurationRecord that specifies
> >>> the decoder configuration information for APV video content
> >>>
> >>> Signed-off-by: Dawid Kozinski <d.kozin...@samsung.com>
> >>> ---
> >>>  libavformat/Makefile    |   2 +-
> >>>  libavformat/apv.c       | 827
> ++++++++++++++++++++++++++++++++++++++++
> >>>  libavformat/apv.h       |  94 +++++
> >>>  libavformat/isom_tags.c |   2 +
> >>>  libavformat/movenc.c    |  47 +++
> >>>  5 files changed, 971 insertions(+), 1 deletion(-)
> >>
> >> Hi,
> >>
> >> Two thoughts here:
> >>
> >> First, your AVPackets contain a raw_bitstream_access_unit().  I don't
> > think this is
> >> the right approach - the packets should contain the codec data only,
> >> not
> > the
> >> additional encapsulation.  (This is the method I followed.)
> >>
> >> For this patch in particular, I think it results in writing the files
> > incorrectly: the
> >> specification says "each sample shall contain one and only one access
> >> unit
> > of
> >> APV coded data", which I interpret to mean one access_unit() syntax
> > structure.
> >>
> >> This also results in the size effectively appearing multiple times in
> >> the
> > file for no
> >> good reason:
> >>
> >> 00000020  66 72 65 65 00 01 15 db  6d 64 61 74 00 01 15 cf
> > |free....mdat....|
> >>
> >>                       ^ mdat size              ^ au_size
> >>
> >> 00000030  61 50 76 31 00 01 15 c7  01 00 01 00 21 21 40 00
> > |aPv1........!!@.|
> >>           ^ signature ^ pbu_size   ^ pbu_type followed by header
> >>
> >> The separate pbu_size makes sense if there is also metadata, but
> >> having
> > the
> >> mdat box with a size immediately followed by the same size (well,
> >> minus
> > twelve
> >> for mdat size + mdat + au size) again inside the box does not seem
> > helpful.
> >>
> >
> > Hi Mark,
> >
> > Indeed. AVPackets contains raw_bitstream_access_unit()
> >
> > raw_bitstream_access_unit() {
> >     au_size                                                   | u(32)
> >     access_unit(au_size)                                      |
> > }
> >
> > Such data comes out from the APV encoder.
> > Data from the encoder can go to the movmuxer, but it can also go to
> > another muxer like apvmuxer.
> >
> > const FFOutputFormat ff_apv_muxer = {
> >     .p.name            = "apv",
> >     .p.long_name       = NULL_IF_CONFIG_SMALL("raw APV video"),
> >     .p.extensions      = "apv",
> >     .p.audio_codec     = AV_CODEC_ID_NONE,
> >     .p.video_codec     = AV_CODEC_ID_APV,
> >     .flags_internal    = FF_OFMT_FLAG_MAX_ONE_OF_EACH |
> >                          FF_OFMT_FLAG_ONLY_DEFAULT_CODECS,
> >     .write_packet      = ff_raw_write_packet,
> >     .p.flags           = AVFMT_NOTIMESTAMPS,
> > };
> >
> > Thanks to the fact that the APV muxer receives such data packed in
> > AVPacket, I was able to use the default function ff_raw_write_packet()
> > from rawenc.c as FFOutputFormat::write_packet for ff_apv_muxer.
> >
> > The APV muxer uses ff_raw_write_packet, which takes the data that came
> > from the encoder packed in AVPacket and writes the data to a file.
> >
> > In the case of saving the elementary APV stream to a file, the data
> > should have the format [au_size (4Bytes)][access_unit][au_size
> > (4Bytes)][access_unit]...[au_size(4Bytes)][access_unit].
> >
> > Regarding APV in movmuxer, we can eliminate the additional AU size in
> > two
> > ways:
> >
> > 1.
> > We can change the output data format from the encoder so that the
> > AVPacket contains access_unit() without the 4-byte prefix containing
> > information about the length of the AU. However, this will require
> > using a custom .write_packet function in ff_apv_muxer that will add a
> > prefix specifying the size of the AU in the output stream before each access
> unit.
> >
> >
> > 2.
> > We will make changes in the mov muxer (movenc.c). We will not write
> > the data using the call avio_write(pb, pkt->data, size); but will
> > implement custom handling for APV (an additional if() in
> ff_mov_write_packet()):
> >
> > if (par->codec_id == AV_CODEC_ID_APV) {
> >     avio_write(pb, pkt->data + 4, size - 4); }
> >
> > Please share your opinion.
> 
> I don't think the implementation details which you mention are relevant to the
> questions I am asking.
> 
> 
> The primary point here is that the spec at
> <https://protect2.fireeye.com/v1/url?k=3154f65d-50dfe36e-31557d12-
> 000babff9bb7-62e68669a9a2e365&q=1&e=25c0300b-6820-427e-9078-
> edf55c3bc81b&u=https%3A%2F%2Fgithub.com%2FAcademySoftwareFoundatio
> n%2Fopenapv%2Fblob%2Fmain%2Freadme%2Fapv_isobmff.md> says:
> 
>   "each sample shall contain one and only one access unit of APV coded data"
> 
> This states that it contains an "access unit", not a "raw bitstream access 
> unit" as
> in your implementation.
> 
> Since your implementation and the spec disagree, can you comment on the
> status / intended finality of each of them?  (Is there another implementation 
> of
> this which you are testing against, or is this the first one?)
> 
> To me the spec definition appears to be the more sensible one, since the raw
> bitstream format functions as a container and it does not make sense to nest
> containers.
> 
> 
> Separately to that queston, there is the point that inside ffmpeg we may make
> our own choice about what an AVPacket should contain (and then use it
> consistently).
> 
> In this case my opinion is that having an AVPacket contain an access unit (the
> top-level syntax structure defined in the "syntax and semantics" section of 
> the
> specification) is the choice which makes most sense, and it is also the one 
> which
> I have followed consistently in my patch series.
> 
Mark, thank you for your quick response and for sharing your insights. I 
appreciate your perspective on the implementation details, and I understand 
your concerns regarding the alignment between the specification and the current 
implementation (or rather, the lack of alignment between the current 
implementation and the spec).

You are right; the specification clearly states that "each sample shall contain 
one and only one access unit of APV coded data," and I agree with you that the 
implementation should be consistent with the specification.

I am currently working on a new version of the implementation that takes into 
account compliance with the specification, in which the AVPacket will contain 
the access unit, but it  requires testing.

> >> Second, I think we need a consistent decision on what the extradata
> >> should
> > be
> >> doing.  The APVDecoderConfigurationRecord makes sense as a thing for
> >> it to contain, but it's not clear to me that it needs to exist at all
> >> as it has
> > no effect on
> >> anything inside ffmpeg (a decoder will always ignore it).
> >>
> >> You currently make extradata from one of your demuxers but not other
> >> one
> > or
> >> the encoder, and nothing requires it when consuming.  Why is it
> >> useful to
> > have
> >> ever?
> 
> Your comment on this question would also be appreciated.
> 
I appreciate your insights regarding extradata.

I agree that we need to establish a consistent approach to how extradata is 
utilized.
I understand your concerns about the necessity of including the 
APVDecoderConfigurationRecord as part of the extradata, especially if it has no 
impact on the decoding process.

The current implementation, where the APVDecoderConfigurationRecord is 
generated by the movmuxer but the extradata is not used by the decoder, and 
nothing currently requires data from the extradata, indeed raises questions 
about its overall utility. 
If extradata is not required for consumption and does not influence the 
decoding process, we should carefully consider its significance.

In my view, if we decide to retain the extradata, we should ensure that it 
serves a clear purpose and is consistently implemented across all components. 
If we determine that it does not provide any significant benefits, it may be 
worth discussing its removal.

BR
Dawid

> Thanks,
> 
> - Mark
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://protect2.fireeye.com/v1/url?k=aab97fc0-cb326af3-aab8f48f-
> 000babff9bb7-6fcdf9423671c6a7&q=1&e=25c0300b-6820-427e-9078-
> edf55c3bc81b&u=https%3A%2F%2Fffmpeg.org%2Fmailman%2Flistinfo%2Fffmp
> eg-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".