improvements - see description

[anthwlock]

fixes:

• readBits not working as expected
• no keyframes
• mdat->end set too early

changes:

• moved the buffer layer to the file class -> >10x faster
• forbidden bit == 1 inidicates the payload is malformed, not the header
• removed dependency on internal libavcodec code (added sps-decoder)
• splitted classes in seperated files
• class members names now use suffix '_'
• build instructions

added:

• regulation of log messages (-v -vv -q)
• default output now usefull for average user

[Hacklin]

You should split this PR into seperate ones, as you do to much in a single PR.

Notes:

• removed dependency on internal libavcodec code (added sps-decoder)

You forgot to remove the dependency on #include <config.h>.

• class members names now use suffix '_'

This is a style change. You shouldn't make a style change without explicit consent of the source maintainer(s). This should be done in a seperate PR. Discerning the class member variables themselves from non-members by adding a suffix _ or prefixing m_ in their declaration is generally a good idea. However, adding a suffix _ to function parameters where you expect a class member to be used, is a bad idea. Please don't do that.

• build instructions

Please note that:

• Till Ubuntu Trusty (14.04LTS): libav* libraries are from the Libav project.
• Since Ubuntu Xenial (16.04LTS): libav* libraries are from the FFmpeg project (again).

So technically, if you don't care about from which project (FFmpeg or Libav) the libav libraries are from, you should refer to the libraries them as libav with a lower case L or better as libav libraries.

• build instructions

The Docker container. Travis-CI has been hijacked to build a Docker executable from git master. Your changes to the Dockerfile don't change this. So you should leave the instructions about the Docker container in the README.md.

Notes:

• I disagree with the current use of the Dockerfile (commit f004f9c, issue #118) Travis-CI should be used to test the integration of any changes to the source, including PRs. As it stands, the current setup just happens to test changes to master only.
• The Dockerfile used in the service docker in .travis.yml should be used to create a build environment in which to test the integration.
• To create Docker executables it's probably better to use a separate git repository. For Docker executables see also:
  1. Debian-based Docker exe: https://github.com/bylevel/docker_untrunc.git (Uses an embedded builder Docker container to reduce the final executable size.)
  2. ArchLinux-based Docker exe: https://github.com/mooyoul/docker-untrunc.git
  3. Ubuntu:12.04-based Docker exe: https://github.com/synctree/docker-untrunc.git
• Yes, changes to the use of Docker in .travis.yml should be done in a separate PR.

[anthwlock]

I agree, it makes little sense that travis is telling us in PRs that the build for master failed. I didn't forget to remove the dependency to #include <config.h>. The travis build-log does't reflect my commits. The changes in 'README.md' and 'Dockerfile' reflect that building libav is not required anymore.

I know the general rule is to prefer many small PRs instead of one big PR. But please consider this:

• This Program is rather small.
• I described all changes I made, and they can easily be reviewed.
• I didnt add any complicated code.
• Most of the changed lines are due to added suffix or splitting logic in seperate files.
• I dont have time to to redo all the stuff I changed just so it looks prettier in the git log.

[anthwlock]

What are your thoughts on the new Dockerfile @Hacklin @brandon-dacrib

[brandon-dacrib]

LGTM nice and clean

[Hacklin]

Like your idea that if you build the docker with an empty context, you use the sources from the git master.

[anthwlock]

Thank you for the review! In AvcConfig the sps_info_ member is initialized to NULL. You should not access it if is_ok == false. Same for Codec and its avc_config_. It makes little sense to always create a video metada member if Codec may represent some audio codec. Also I dont see the point in accessing last_frame_was_idr_ before you decode a video frame. I would be glad if you could test the code on some real video files.

[Hacklin]

You should not access it if ...

And that is a problem. sps_info_ is a public member. Meaning that anyone at anytime can access and change it. If one should not access it if is_ok == false, then make it so by using an accessor method or at least put a comment in the sources explaining this. Same thing with last_frame_was_idr_. If it's only valid after you decode a video frame, you should make it so by code or by comment.

• This is not a code correctness issue (the code works).
• This is a data encapsulation issue and maintenance issue (the dependency between members is not documented by code nor by comment).

Besides, when avc_config_->is_ok == false, you don't use avc_config_. So, when you check avc_config_->is_ok right after avc_config_ = new AvcConfig(*stsd);, you could just delete avc_config_ and assign nullptr to it. Later on, you then check if it's valid with if(avc_config_).

Sorry. I know these ain't your doing:

• The Untrunc memory management is leaky (missing deletes).
• The Untrunc use of the FFmpeg/Libav API not always correct (wrong *free*() functions).
• The Untrunc OOP isn't good at encapsulation.

These things just irk me. I'm sorry if I'm being to harsh on you.

[Hacklin]

anthwlock, Have you decided on the C++ standard you're gonna use?

• If C++98, then you should rewrite your logging code.
• If C++11 (or later), then might I suggest the use of std::unique_ptr.

[Hacklin]

Style. Your sources have a space after if ( and while (, Ponchio's sources have no space after if( and while(.

[Hacklin]

Why are you using uchar and uint instead of unsigned char and unsigned? Just because it is less to type? (Which is not a good reason for an interface.)

[Hacklin]

The swap*() functions should be defined static inline in the header file, so the compiler can optimize them (to a single instruction).

Note:

• Defining a function in a header file will have a C++ compiler try to inline that function.
• The inline is a hint/request to the C++ compiler, that you really want a function inlined. It depends on the compiler and the optimization level if a function is actual inlined, but usually functions marked with inline will already be inlined at a lower optimization level (-O1).
• The static tells the C++ compiler to treat the function as local to the translation unit (TU); i.e. totally independent from the ones in other TUs (allowed to access the TU globals, the static variables inside the function are different for each TU, when taking the function's address it will be different in each TU).
• The static inline tells the compiler to treat the function as a #define macro. You can assume that the function is always inlined at any optimization level. You cannot take the address of a static inline function.

Also, the compiler will automatically determine the C++11 noexcept status for inline functions, but uses noexcept(false) for non-inline functions as the compiler must assume that a non-inline function can throw exceptions.

[anthwlock]

If you have the time for this feel free to open a pull request. I would be happy to merge them.

[Hacklin]

I'm creating maintenance Pull Requests for the upstream ponchio/untrunc/ repository.

If you want me to do the same for your repository, then I want to know at least 2 things:

• Is your repository a "hard fork"? That is, are you willing to maintain it as an alternative to the original Ponchio's Untrunc? You can then also set the style (tabs, spaces, {}, etc.) of the sources.
• What is the C++ standard you use? Your code uses C++11 features, so it's C++11 or C++14. C++17 maybe a bit too new.

Note that I'm not willing to dive into the functionality of the code itself. I don't want to spend too much time on this and Untrunc is simply too broken (try: untrunc good.mp4 good.mp4 -> the resulting good.mp4_fixed.mp4 is broken).

[Hacklin]

I can confirm that the resulting untrunc binary from this PR works as well as the one from Ponchio's Untrunc master branch.

[anthwlock]

Until @ponchio merges this PR I will keep it up as as "hard fork". Use C++14 and tabs for indentation otherwise google's C++ style. You tested untrunc good.mp4 good.mp4 on mine too? I think it works. All it should do is making good.mp4 streamable by moving the moov-Atom to the top of the file. Although there is a unused explicit function for that... Mp4::makeStreamable.

[Hacklin]

Yes doing untrunc good.mp4 good.mp4 with your Untrunc works. Thank you, your Untrunc version is definitely better than the one from Ponchio's master.

[mqudsi]

Thanks for maintaining this branch/pr.

If built against ffmpeg N-93087-g2b8458fcc5, this branch fails in the call to avformat_open_input() which returns (-1094995529) (invalid data while processing header); but I can't reproduce that issue with ponchio/master. Building this branch against 3.4.4-0ubuntu0.18.04.1 instead works fine.

failed version:

ffmpeg version N-93087-g2b8458fcc5 Copyright (c) 2000-2019 the FFmpeg developers
built with clang version 8.0.0-svn346335-1~exp1+0~20181107174055.608~1.gbp6afd8e (trunk)
configuration: --enable-libfdk-aac --enable-nonfree --cc=clang-8 --cxx=clang++-8 --enable-version3 --enable-static --disable-debug --disable-ffplay --disable-indev=sndio --disable-outdev=sndio --enable-fontconfig --enable-frei0r --enable-gnutls --enable-gray --enable-libfribidi --enable-libass --enable-libvmaf --enable-libfreetype --enable-libmp3lame --enable-libopenjpeg --enable-librubberband --enable-libsoxr --enable-libspeex --enable-libvorbis --enable-libopus --enable-libtheora --enable-libvidstab --enable-libvpx --enable-libwebp --enable-libx264 --enable-libx265 --enable-libxml2 --enable-libxvid --enable-libvidstab --enable-gpl --enable-libaom --enable-muxer=m4v --enable-libzimg --disable-libvmaf
libavutil      56. 26.100 / 56. 26.100
libavcodec     58. 46.100 / 58. 46.100
libavformat    58. 26.100 / 58. 26.100
libavdevice    58.  6.101 / 58.  6.101
libavfilter     7. 48.100 /  7. 48.100
libswscale      5.  4.100 /  5.  4.100
libswresample   3.  4.100 /  3.  4.100
libpostproc    55.  4.100 / 55.  4.100

ok version:

ffmpeg version 3.4.4-0ubuntu0.18.04.1 Copyright (c) 2000-2018 the FFmpeg developers
built with gcc 7 (Ubuntu 7.3.0-16ubuntu3)
configuration: --prefix=/usr --extra-version=0ubuntu0.18.04.1 --toolchain=hardened --libdir=/usr/lib/x86_64-linux-gnu --incdir=/usr/include/x86_64-linux-gnu --enable-gpl --disable-stripping --enable-avresample --enable-avisynth --enable-gnutls --enable-ladspa --enable-libass --enable-libbluray --enable-libbs2b --enable-libcaca --enable-libcdio --enable-libflite --enable-libfontconfig --enable-libfreetype --enable-libfribidi --enable-libgme --enable-libgsm --enable-libmp3lame --enable-libmysofa --enable-libopenjpeg --enable-libopenmpt --enable-libopus --enable-libpulse --enable-librubberband --enable-librsvg --enable-libshine --enable-libsnappy --enable-libsoxr --enable-libspeex --enable-libssh --enable-libtheora --enable-libtwolame --enable-libvorbis --enable-libvpx --enable-libwavpack --enable-libwebp --enable-libx265 --enable-libxml2 --enable-libxvid --enable-libzmq --enable-libzvbi --enable-omx --enable-openal --enable-opengl --enable-sdl2 --enable-libdc1394 --enable-libdrm --enable-libiec61883 --enable-chromaprint --enable-frei0r --enable-libopencv --enable-libx264 --enable-shared
libavutil      55. 78.100 / 55. 78.100
libavcodec     57.107.100 / 57.107.100
libavformat    57. 83.100 / 57. 83.100
libavdevice    57. 10.100 / 57. 10.100
libavfilter     6.107.100 /  6.107.100
libavresample   3.  7.  0 /  3.  7.  0
libswscale      4.  8.100 /  4.  8.100
libswresample   2.  9.100 /  2.  9.100
libpostproc    54.  7.100 / 54.  7.100

[anthwlock]

@mqudsi I am unable to reproduce this with N-93826-gab648f79c8. For further discussion please open an issue on my fork!