[FFmpeg-devel] [PATCH 2/2] avformat/movenc: parse h264 packets to build Sync Sample and Recovery Point tables

[Michael Niedermayer]

On Fri, Jul 30, 2021 at 10:31:53AM -0300, James Almer wrote:
> On 7/30/2021 8:33 AM, Michael Niedermayer wrote:
> > On Thu, Jul 29, 2021 at 03:09:03PM -0300, James Almer wrote:
> > > On 7/29/2021 2:58 PM, Michael Niedermayer wrote:
> > > > On Tue, Jul 27, 2021 at 10:08:20AM -0300, James Almer wrote:
> > > > > Since we can't blindly trust the keyframe flag in packets and assume its
> > > > > contents are a valid Sync Sample, do some basic bitstream parsing to build the
> > > > > Sync Sample table in addition to a Random Access Recovery Point table.
> > > > > 
> > > > > Suggested-by: ffmpeg at fb.com
> > > > > Signed-off-by: James Almer <jamrial at gmail.com>
> > > > > ---
> > > > >    libavformat/movenc.c         | 125 +++++++++++++++++++++++++++++++++--
> > > > >    libavformat/movenc.h         |   1 +
> > > > >    tests/ref/lavf-fate/h264.mp4 |   6 +-
> > > > >    3 files changed, 123 insertions(+), 9 deletions(-)
> > > > 
> > 
> > A.
> > > > Will this allow a user to mark a subset of keyframes as random
> > > > access points ?
> > > > A user may want seeking to preferably happen to a scene start and not a
> > > > frame before
> > > > 
> > 
> > B.
> > > > Will this allow a user to mark a damaged keyframe as such ?
> > > > A frame might in fact have been a keyframe and maybe the only in the file
> > > > but maybe was damaged so a parser might fail to detect it.
> > > 
> > > This code will ensure all packets with an IDR picture will be listed in the
> > > Sync Sample table, and all packets with a Recovery Point SEI message will be
> > > listed in the Random Access Recovery Point table.
> > > Whatever is signaled in packets (like the keyframe flag) is ignored to
> > > prevent creating non-spec compliant output.
> > 
> > The file in case B will be non compliant, it is damaged data, it cannot
> > be muxed in a compliant way. If we support muxing damaged data then
> > parsing that data as a way to unconditionally override the user is
> > problematic.
> > If you try to interpret damaged data the result is undefined, the spec
> > does not tell you how to interpret it and 2 parsers can disagree
> > if a damaged frame is a keyframe.
> 
> If you don't mark a packet as a Sync Sample, even if it contains damaged
> data, the file is still compliant. The point here is to avoid filling the
> Sync Sample table with bogus entries.

IMHO, damaged data is not always a compliant video stream, otherwise in the most extreem
example /dev/random would have to be a compliant h264 stream.


> 
> > 
> > Lets just as 2 examples consider multiple slices some being parsed as IDR
> > some as non IDR, so what is that ? or what if some packet reodering or
> > duplication causes parts of a IDR and non IDR frame to be mixed.
> > where is the IDR frame in that case ? 2 parsers can disagree
> 
> The AVCodecParserContext will tag a packet with an IDR slice as keyframe,
> and then disregard anything else it may find. I made the code i wrote here
> do the same.

If thats the case, we could take the example of a frame lets say it has
100 slices, and in one of these 1 byte is changed so it becomes a IDR slice
That is not really a keyframe or sync symple yet it will
be marked as one IIUC.
Now to continue with this example, which way will it work better
X. With this marked not a keyframe or
Y. With this marked as keyframe as the parser detects ?

Its not containing a single valid IDR slice just 1 of the non IDR slices
have been damaged and are misparsed. That will not work well as a sync sample
yet as a normal sample the 1% of damage might not be noticed at all as its copied
from the last frame



> 
> > still a decoder can with some luck start decoding from such a
> > point i would guess. But does the user want this marked as a keyframe ?
> > maybe yes, maybe no. I think taking the users only way to set the keyframe
> > flag away is a step backward
> 
> Look at MPEG2 parsing and others in this container. It's the same scenario.
> Did any of these concerns show up back then?
> What I'm doing here for h264 is the same thing we did for those, to ensure
> we don't create non spec compliant files because of damaged or mistagged
> input, or a careless user.
> 
> Letting the user mark whatever they want as a Sync Sample on a container
> where it's explicit what should and should not be such is not good practice.
> If you want that, you can use Matroska and other formats where keyframe
> tagging is apparently not strict.
> 
> Now, i want to point out that I wrote this parsing code here because my
> previous attempt at stopping the AVCodecParserContext from being too liberal
> marking things as keyframes was rejected *precisely* because it would let
> the user create non compliant output in mp4. And now you don't want this one
> either because you want to let the user create non complaint output.
> I'm running in circles here and it's getting tiresome.

I see the problem and i agree with you that it is a problem that needs
a solution. Its very bad if the users sets bad flags and it results in
non-compliant files. What my concern is, is that theres no way for the
user to override this when its the parsing code thats wrong and previously
the user could override this.
It may be very rare that the parser is wrong and a user would bother to
override it, i do not know, then again some people are quite crazy with
the level of perfection they try to achieve in their videos

Theres also the 2nd situation where all the information the parser
extracts is already known to the lib user and going over the whole
bitstream is wasting cpu cycles. This can be a situation where for
example all input files where just a moment ago created by FFmpeg
and we assume these would then be as compliant as redoing the parsing
would make them

Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210731/b2c0aaad/attachment.sig>