[FFmpeg-devel] Regarding the removal of hacks

[Michael Niedermayer]

On Sat, Mar 10, 2018 at 03:08:16PM +0200, Jan Ekström wrote:
> Hi,
> 
> I would like to raise a question regarding how the community feels
> about removing hacks which most likely were done to work around
> deficiencies of API clients. Silently.

Iam in favor of removing hacks and replacing them by better
solutions.

To do this its important to first understand why a hack was needed
and what it does. Otherwise no correct or better implementation is
possible and removial would just add back a bug.


> 
> Some weeks ago someone asked on #ffmpeg why he wasn't getting the time
> scale (time base in MOV/ISOBMFF speak) he wanted out of movenc.c. So I
> looked into it:
> 
> 1) https://git.videolan.org/?p=ffmpeg.git;a=commit;h=b02493e47668e66757b72a7163476e590edfea3a
>   (2012) Some API clients didn't clearly set a time base correctly for
> VFR content - a hack was added in movenc.c to bump the time scale to
> at least 10k.

Iam not sure this was related to VFR as you write here.

I think what this was about was A/V desync on more than VFR videos

to illustrate, consider
that if you create a slide show with 1fps you get upto
1 second desync if you are unlucky. 

the same happens with normal frame rates in CFR video.
That is this fixed the case where a audio and video sample needed to be
synced exactly and not by upto 1/30 only (for 30fps video)

This problem was IIRC interlinked with the need to shift initial negative
DTS into the zero or positive.
So even if an application supplied exact timestamps in a low timebase like
1/30. after the shift to get DTS after 0 they would no longer be where they
where before when  the timebases where not accurate enough.
So in from the applications point of view 1/30 could be fully exact and good
enough yet because dts where negative and needed to be shifted a more precisse
timebase was needed to do this without introducing AV desync.

This issue may have been fixed with edit lists



> 2) https://git.videolan.org/?p=ffmpeg.git;a=commit;h=7e570f027b9bb0b5504
> ed443c70ceb654930859d
>   (2013) Someone clearly didn't like this forced behavior so an
> AVOption was added to set a forced video time base for ALL video
> tracks in a mux.

> 3) https://git.videolan.org/?p=ffmpeg.git;a=commit;h=194be1f43ea391eb986732707435176e579265aa
>   (2014) Anton switches the time base utilized from the encoder
> AVCodec to the AVStream, which most likely fixes the issue of output
> time base being incorrect as some encoders dislike weird (?) time
> bases (although how would you get VFR content through those if that
> was the reason is still beyond me).

> 
> After this I told the user that this is a hack that's been added and
> that his timestamps are still exact (which they are, just that the
> time base is larger than it is). But as a developer, this just looks
> like the whole mess should be removed:
> 
> 1) ffmpeg.c , for which the original hack possibly was added in 2012,
> now has an option to set output time base as far as I can remember? So
> the user of the tool can properly set a specific time base if
> required, just like with the AVOption.
> 2) This is just extra complexity/automated logic that can be hiding
> bugs in API clients, and we ended up having an additional AVOption.
> 

> So, going forward, would patches to remove things like these be worth
> sending to the list?

yes, but in the more general case
it could require a bit of care and could in some cases be alot
more work than it initially looks like.
It also would likely often require quite a bit of testing and figuring
out what the "to be removed" code exactly was needed for.
That is testing, code archiology, debuging, and where hacks still are needed
design and implementation of better solutions.

thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180310/8912389b/attachment.sig>