This repository has been archived by the owner. It is now read-only.

support colr atom #1804

Merged
merged 1 commit into from Apr 22, 2017

Conversation

3 participants
@chenchao1983

Support colr atom for quicktime mp4 container

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus Nov 7, 2016

Owner

Thanks for the updated pull request. I'll leave notes inline.

A general note though: your git repository is not up to date regarding the libebml and libmatroska submodules. Therefore they show up as changes in your pull request. Neither of those should actually be part of this pull request.

Please update your submodules re-base this pull request (together with the other changes I'll ask about). Thanks.

Owner

mbunkus commented Nov 7, 2016

Thanks for the updated pull request. I'll leave notes inline.

A general note though: your git repository is not up to date regarding the libebml and libmatroska submodules. Therefore they show up as changes in your pull request. Neither of those should actually be part of this pull request.

Please update your submodules re-base this pull request (together with the other changes I'll ask about). Thanks.

src/input/r_qtmp4.cpp
+ if (new_dmx->colour_matrix_coefficients != 2) {
+ m_ti.m_colour_matrix.set(new_dmx->colour_matrix_coefficients,
+ OPTION_SOURCE_CONTAINER);
+ }

This comment has been minimized.

@mbunkus

mbunkus Nov 7, 2016

Owner

In all three cases: please don't line-break. I don't work with 80 column limits.

@mbunkus

mbunkus Nov 7, 2016

Owner

In all three cases: please don't line-break. I don't work with 80 column limits.

src/input/r_qtmp4.cpp
+ if (stsd_non_priv_struct_size < atom_size) {
+ auto offset = stsd_non_priv_struct_size;
+ auto atom_ptr = priv + offset;
+ uint32_t size = get_uint32_be(atom_ptr);

This comment has been minimized.

@mbunkus

mbunkus Nov 7, 2016

Owner

Please align these on =.

@mbunkus

mbunkus Nov 7, 2016

Owner

Please align these on =.

src/input/r_qtmp4.cpp
+ }
+}
+
+void qtmp4_demuxer_c::handle_colr_atom(uint64_t offset, uint64_t size) {

This comment has been minimized.

@mbunkus

mbunkus Nov 7, 2016

Owner

Please use the function declaration style that I use in other places, too.

@mbunkus

mbunkus Nov 7, 2016

Owner

Please use the function declaration style that I use in other places, too.

src/input/r_qtmp4.cpp
+ colr_atom_t colr_atom;
+ size_t color_atom_size = sizeof(colr_atom_t);
+ if (sizeof(color_atom_size) > size)
+ mxerror(boost::format(Y("Quicktime/MP4 reader: Could not read the colour atom for track ID %1%.\n")) % id);

This comment has been minimized.

@mbunkus

mbunkus Nov 7, 2016

Owner

mxerror will abort the program. This is not acceptable for such a case. If the colour atom cannot be parsed then this warrants a warning at most, but the process must continue without setting the colour header fields.

@mbunkus

mbunkus Nov 7, 2016

Owner

mxerror will abort the program. This is not acceptable for such a case. If the colour atom cannot be parsed then this warrants a warning at most, but the process must continue without setting the colour header fields.

src/input/r_qtmp4.cpp
+ if (colour_type == "nclc") {
+ colour_primaries = get_uint16_be(&colr_atom.colour_primaries);
+ colour_transfer_characteristics =
+ +get_uint16_be(&colr_atom.transfer_characteristics);

This comment has been minimized.

@mbunkus

mbunkus Nov 7, 2016

Owner

Again no line break here, please.

@mbunkus

mbunkus Nov 7, 2016

Owner

Again no line break here, please.

This comment has been minimized.

@mbunkus

mbunkus Nov 7, 2016

Owner

And align on =, please.

@mbunkus

mbunkus Nov 7, 2016

Owner

And align on =, please.

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus Dec 13, 2016

Owner

Just a friendly question: are you going to continue working on this pull request?

Owner

mbunkus commented Dec 13, 2016

Just a friendly question: are you going to continue working on this pull request?

@chenchao1983

This comment has been minimized.

Show comment
Hide comment
@chenchao1983

chenchao1983 Dec 13, 2016

Yes, I will keep working on this request. I am a bit busy these days. I will try to fix it next week.

Yes, I will keep working on this request. I am a bit busy these days. I will try to fix it next week.

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus Dec 13, 2016

Owner

That's good to hear, thanks. Take your time; the pull request just seemed to be abandoned, and I wanted to get some clarity.

Owner

mbunkus commented Dec 13, 2016

That's good to hear, thanks. Take your time; the pull request just seemed to be abandoned, and I wanted to get some clarity.

@chenchao1983

This comment has been minimized.

Show comment
Hide comment
@chenchao1983

chenchao1983 Jan 2, 2017

Dear Moritz
I have fixed the comments. Please have another look.

happy new year.

Dear Moritz
I have fixed the comments. Please have another look.

happy new year.

@mbunkus

Thanks for the push. Apart from the line comments that I've added there's also the issue that your commit still updates the libebml and libmatroska sub-modules. The commit shouldn't modify them at all for this functionality.

src/input/r_qtmp4.cpp
@@ -1227,6 +1227,15 @@ qtmp4_reader_c::handle_stsd_atom(qtmp4_demuxer_cptr &new_dmx,
mxerror(boost::format(Y("Quicktime/MP4 reader: Could not read the stream description atom for track ID %1%.\n")) % new_dmx->id);
new_dmx->handle_stsd_atom(size, level);
+ if (new_dmx->colour_primaries != 2) {

This comment has been minimized.

@mbunkus

mbunkus Jan 3, 2017

Owner

This is the wrong way to transfer the information from the reader to the packetizer. The problem is that there's only a single m_ti instance that is handed to all packetizers. Therefore setting fields in it will transfer the same information not just to the video packetizer, but to all other packetizers created from the MP4 file afterwards.

A better way looks like this:

  1. Create a new function in qtmp4_demuxer_c that sets the colour values on a newly created packetizer. In it use function calls like m_reader.m_reader_packetizers[ptzr]->set_video_colour_primaries(colour_primaries, OPTION_SOURCE_CONTAINER); and similar ones for the other two properties.
  2. In qtmp4_reader_c::create_packetizer there's a call dmx->set_packetizer_display_dimensions(). Right after that call you can call that new function created in step 1.
@mbunkus

mbunkus Jan 3, 2017

Owner

This is the wrong way to transfer the information from the reader to the packetizer. The problem is that there's only a single m_ti instance that is handed to all packetizers. Therefore setting fields in it will transfer the same information not just to the video packetizer, but to all other packetizers created from the MP4 file afterwards.

A better way looks like this:

  1. Create a new function in qtmp4_demuxer_c that sets the colour values on a newly created packetizer. In it use function calls like m_reader.m_reader_packetizers[ptzr]->set_video_colour_primaries(colour_primaries, OPTION_SOURCE_CONTAINER); and similar ones for the other two properties.
  2. In qtmp4_reader_c::create_packetizer there's a call dmx->set_packetizer_display_dimensions(). Right after that call you can call that new function created in step 1.
src/input/r_qtmp4.cpp
@@ -2620,6 +2629,34 @@ qtmp4_demuxer_c::handle_video_stsd_atom(uint64_t atom_size,
v_width = get_uint16_be(&v_stsd.width);
v_height = get_uint16_be(&v_stsd.height);
v_bitdepth = get_uint16_be(&v_stsd.depth);
+ // Handle Extensions
+ if (stsd_non_priv_struct_size < atom_size) {

This comment has been minimized.

@mbunkus

mbunkus Jan 3, 2017

Owner

This comparison should at least check for enough space for the 8-byte reads that follow (one for the size field, one for the FourCC field), e.g. if ((stsd_non_priv_struct_size + 8) <= atom_size).

@mbunkus

mbunkus Jan 3, 2017

Owner

This comparison should at least check for enough space for the 8-byte reads that follow (one for the size field, one for the FourCC field), e.g. if ((stsd_non_priv_struct_size + 8) <= atom_size).

src/input/r_qtmp4.cpp
+ uint32_t size = get_uint32_be(atom_ptr);
+ fourcc_c ext_fourcc{atom_ptr + sizeof(uint32_t)};
+ if (ext_fourcc == "colr") {
+ handle_colr_atom(offset, size);

This comment has been minimized.

@mbunkus

mbunkus Jan 3, 2017

Owner

Here a check that the size just read must not exceed the atom's size should be added in order to avoid a potential buffer overflow.

@mbunkus

mbunkus Jan 3, 2017

Owner

Here a check that the size just read must not exceed the atom's size should be added in order to avoid a potential buffer overflow.

src/input/r_qtmp4.h
@@ -255,6 +255,7 @@ struct qtmp4_demuxer_c {
memory_cptr stsd;
unsigned int stsd_non_priv_struct_size;
uint32_t v_width, v_height, v_bitdepth, v_display_width_flt{}, v_display_height_flt{};
+ uint16_t colour_primaries, colour_transfer_characteristics, colour_matrix_coefficients;

This comment has been minimized.

@mbunkus

mbunkus Jan 3, 2017

Owner

Can you please rename the three variables and prefix each with v_ similar to v_width etc.? Thanks.

@mbunkus

mbunkus Jan 3, 2017

Owner

Can you please rename the three variables and prefix each with v_ similar to v_width etc.? Thanks.

@chenchao1983

This comment has been minimized.

Show comment
Hide comment
@chenchao1983

chenchao1983 Apr 13, 2017

Hi, Moritz
I am working on this feature but was blocked by building.

When I do ./configure
I always see this error

1203 configure:10624: x86_64-grtev4-linux-gnu-g++ -c -I/usr/local/include  -I/usr/include conftest.cpp >&5
1204 In file included from /usr/local/include/boost/lexical_cast/detail/inf_nan.hpp:34:0,
1205                  from /usr/local/include/boost/lexical_cast/detail/converter_lexical_streams.hpp:63,
1206                  from /usr/local/include/boost/lexical_cast/detail/converter_lexical.hpp:54,
1207                  from /usr/local/include/boost/lexical_cast/try_lexical_convert.hpp:42,
1208                  from /usr/local/include/boost/lexical_cast.hpp:32,
1209                  from conftest.cpp:75:
1210 /usr/local/include/boost/math/special_functions/sign.hpp: In function 'int boost::math::detail::signbit_impl(T, const boost::math::detail::native_tag&)':
1211 /usr/local/include/boost/math/special_functions/sign.hpp:30:17: error: 'signbit' is not a member of 'std'
1212          return (std::signbit)(x);

I have installed boost 1.63.0 but the problem still persists.

Could you have a look? Is it related to compiler? my boost is built and installed using gcc-4.8 but the project requires gcc-4.9.

thanks,
Chao Chen

chenchao1983 commented Apr 13, 2017

Hi, Moritz
I am working on this feature but was blocked by building.

When I do ./configure
I always see this error

1203 configure:10624: x86_64-grtev4-linux-gnu-g++ -c -I/usr/local/include  -I/usr/include conftest.cpp >&5
1204 In file included from /usr/local/include/boost/lexical_cast/detail/inf_nan.hpp:34:0,
1205                  from /usr/local/include/boost/lexical_cast/detail/converter_lexical_streams.hpp:63,
1206                  from /usr/local/include/boost/lexical_cast/detail/converter_lexical.hpp:54,
1207                  from /usr/local/include/boost/lexical_cast/try_lexical_convert.hpp:42,
1208                  from /usr/local/include/boost/lexical_cast.hpp:32,
1209                  from conftest.cpp:75:
1210 /usr/local/include/boost/math/special_functions/sign.hpp: In function 'int boost::math::detail::signbit_impl(T, const boost::math::detail::native_tag&)':
1211 /usr/local/include/boost/math/special_functions/sign.hpp:30:17: error: 'signbit' is not a member of 'std'
1212          return (std::signbit)(x);

I have installed boost 1.63.0 but the problem still persists.

Could you have a look? Is it related to compiler? my boost is built and installed using gcc-4.8 but the project requires gcc-4.9.

thanks,
Chao Chen

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus Apr 14, 2017

Owner

I don't have systems with such old gcc versions anymore, and I've never encountered this error myself. I don't think I can really help you with this.

You absolutely need to use gcc 4.9 or newer in order to compile MKVToolNix. The source code uses features that gcc 4.8 simply doesn't support.

Owner

mbunkus commented Apr 14, 2017

I don't have systems with such old gcc versions anymore, and I've never encountered this error myself. I don't think I can really help you with this.

You absolutely need to use gcc 4.9 or newer in order to compile MKVToolNix. The source code uses features that gcc 4.8 simply doesn't support.

@chenchao1983

This comment has been minimized.

Show comment
Hide comment
@chenchao1983

chenchao1983 Apr 17, 2017

Hi, Moritz
I have addressed your comments. Please have another look.

best,
Chao Chen

Hi, Moritz
I have addressed your comments. Please have another look.

best,
Chao Chen

@mbunkus mbunkus merged commit 4bfc056 into mbunkus:master Apr 22, 2017

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus Apr 22, 2017

Owner

Thanks. I've merged this version and fixed a couple of issues in cd742ed. It'll be part of the next release.

Owner

mbunkus commented Apr 22, 2017

Thanks. I've merged this version and fixed a couple of issues in cd742ed. It'll be part of the next release.

mbunkus added a commit that referenced this pull request Apr 22, 2017

@chenchao1983

This comment has been minimized.

Show comment
Hide comment
@chenchao1983

chenchao1983 Apr 23, 2017

@galad87

This comment has been minimized.

Show comment
Hide comment
@galad87

galad87 May 12, 2017

ISO File Format (and mp4) uses colour_type == "nclx" instead of "nclc".

galad87 commented May 12, 2017

ISO File Format (and mp4) uses colour_type == "nclx" instead of "nclc".

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus May 14, 2017

Owner

@galad87 Do you have a sample file for me to test with? If so, please upload it to my FTP server or give me an URL to download from. Thanks.

Owner

mbunkus commented May 14, 2017

@galad87 Do you have a sample file for me to test with? If so, please upload it to my FTP server or give me an URL to download from. Thanks.

@galad87

This comment has been minimized.

Show comment
Hide comment
@galad87

galad87 May 14, 2017

Uploaded test.mp4

galad87 commented May 14, 2017

Uploaded test.mp4

@mbunkus

This comment has been minimized.

Show comment
Hide comment
@mbunkus

mbunkus May 14, 2017

Owner

Thanks.

Owner

mbunkus commented May 14, 2017

Thanks.

mbunkus added a commit that referenced this pull request May 14, 2017

MP4 reader: find `colr` atom even if it's not first `stsd` atom exten…
…sion

Another fix for the functionality introduced in #1804.

mbunkus added a commit that referenced this pull request May 14, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.