GNOME Bugzilla – Bug 769048
qtmux: prores-related fixes
Last modified: 2016-11-01 18:46:07 UTC
qtmux: Write COLR and FIEL atoms for prores video ========================== qtmux: Fix fourcc for ProRes Proxy This is apco, according to https://wiki.multimedia.cx/index.php?title=Apple_ProRes
Created attachment 331895 [details] [review] 0001-qtmux-Write-COLR-and-FIEL-atoms-for-prores-video.patch
Created attachment 331896 [details] [review] 0002-qtmux-Fix-fourcc-for-ProRes-Proxy.patch
Note that this is still WIP. The encoder needs to provide the correct caps in order for the muxer to write the necessary information. This isn't done yet, it's only been tested with capssetter.
Review of attachment 331896 [details] [review]: ::: gst/isomp4/gstqtmux.c @@ +4204,3 @@ entry.fourcc = FOURCC_apch; else if (!g_strcmp0 (variant, "proxy")) + entry.fourcc = FOURCC_apco; And what's ap4h? It should probably be another variant and be used here :) 4:4:4 "high" or something?
Created attachment 334015 [details] [review] 0001-qtmux-Write-additional-atoms-for-prores-video.patch New patches attached. Please review and feedback. There is no information on the caps about whether the interlaced content is TFF or BFF. I added a custom top-fields-first boolean which means that all prores encoders will have to provide this field. Currently this information is on each frame and not on the caps, which might be a bit late in our case (or at least, make the implementation more complicated). If you have a better idea, please let me know. Note that, on the libav side, there's no work done yet for top vs bottom fields first, we just assume it's TFF until there's a final decision on how to handle this.
Created attachment 334016 [details] [review] 0002-Fix-fourcc-for-ProRes-Proxy.-This-is-apco-according-.patch
Created attachment 334017 [details] [review] 0001-avvidenc-Send-colorimetry-and-interlacing-informatio.patch This patch is for gst-libav.
Reference file: https://ahiru.eu/~vivia/reference.mov
Review of attachment 334015 [details] [review]: Thanks for the patches, just some minor comments. ::: gst/isomp4/atoms.c @@ +4026,3 @@ + par_n = entry->par_n; + par_d = entry->par_d; Is this guaranteed to be non-zero or are we risking setting 0:0 as par? ::: gst/isomp4/gstqtmux.c @@ +4209,3 @@ + !g_strcmp0 (interlace_mode, "mixed")) { + /* Assume top-fields-first if unspecified */ + gst_structure_get_boolean (structure, "top-field-first", &tff); are there more types than top-field-first and bottom-field-first? Maybe it would be better to have a string? Does interleave/deinterleave have any caps for this already? @@ +4220,3 @@ + "Colorimetry information not found in caps. The resulting file's " + "color information might be wrong"); + ext_atom = build_fiel_extension_prores (interlace_mode, tff); Is this mandatory? Do we have to guess? Anyway this is only used for prores so it shouldn't matter for other formats.
(In reply to Thiago Sousa Santos from comment #9) > ::: gst/isomp4/gstqtmux.c > @@ +4209,3 @@ > + !g_strcmp0 (interlace_mode, "mixed")) { > + /* Assume top-fields-first if unspecified */ > + gst_structure_get_boolean (structure, "top-field-first", &tff); > > are there more types than top-field-first and bottom-field-first? Maybe it > would be better to have a string? Does interleave/deinterleave have any caps > for this already? This is part of the problem :) TFF/BFF is something we don't signal in caps but only as buffer flags. I'm thinking of making it (optionally) part of the caps for interlace-mode=interleaved.
commit 25526ed7f3a6d289573aa80a481f9c337ec648b3 Author: Georg Lippitsch <glippitsch@toolsonair.com> Date: Tue Jul 12 18:14:52 2016 +0200 qtmux: Fix fourcc for ProRes Proxy This is apco, according to https://wiki.multimedia.cx/index.php?title=Apple_ProRes https://bugzilla.gnome.org/show_bug.cgi?id=769048
Bug #771376 is a dependency of this, we should solve the TFF/BFF issue generally for video caps, also see comment 10 here.
Created attachment 336461 [details] [review] qtmux: Write additional atoms for prores video These required atoms are: colorimetry, field information, spatial/temporal quality, and vendor.
Review of attachment 336461 [details] [review]: ::: gst/isomp4/atoms.c @@ +3917,3 @@ +AtomInfo * +build_fiel_extension_prores (const gchar * interlace_mode, gboolean tff) This one should be merged with the one for JPEG2000. It is a standard MOV atom that is also relevant for other codecs, and we should always write it for MOV if we know a field order or about interlacing The one for JPEG2000 is also wrong (it only writes one byte, and it has a off-by-one in that byte...) This one depends on the other bug, we should proxy this in a generic way @@ +3935,3 @@ + field_order = tff ? 9 : 14; + else if (!g_strcmp0 (interlace_mode, "mixed")) + field_order = tff ? 1 : 6; This is wrong, "mixed" does not mean that. AFAIU 1 and 6 are the same as 9 and 14, just that the fields are spatially flipped (bottom field is first line). We don't support that at all currently (ffmpeg does). @@ +3948,3 @@ + +AtomInfo * +build_colr_extension (GstVideoColorimetry colorimetry) This is a standard MP4 atom, we should probably always write it if we can. We should proxy the colorimetry on compressed caps too, see the other bug @@ +4003,3 @@ + + /* colour specification box */ + GST_WRITE_UINT32_LE (data, FOURCC_nclc); This should be nclx for MP4, nclc for MOV/etc @@ +4008,3 @@ + GST_WRITE_UINT16_BE (data + 6, transfer_function); /* transfer function */ + GST_WRITE_UINT16_BE (data + 8, matrix); + For MP4 we also need to write the color range at the end: 0_255 is 0x80, 16_235 is 0x00 (all but the first bit are reserved so far) ::: gst/isomp4/gstqtmux.c @@ +4213,3 @@ variant = gst_structure_get_string (structure, "variant"); + colorimetry_str = gst_structure_get_string (structure, "colorimetry"); + interlace_mode = gst_structure_get_string (structure, "interlace-mode"); We should do all this generally for all video codecs
Created attachment 336464 [details] [review] qtmux: Write additional atoms for prores video These required atoms are: colorimetry, field information, spatial/temporal quality, and vendor.
Created attachment 336465 [details] [review] atoms: 'pasp' atom is also part of MP4, write it always
Review of attachment 336464 [details] [review]: As before, needs some more work. We also need to write the 'clap' atom (which is also part of MP4), if we can.
Comment on attachment 334017 [details] [review] 0001-avvidenc-Send-colorimetry-and-interlacing-informatio.patch This one will become obsolete once the proxying is done in GstVideoEncoder/Decoder
Review of attachment 334017 [details] [review]: Seems even more wrong, some of the colorimetry information is found in the encoded bitstream and should supersede what the container say.
(In reply to Nicolas Dufresne (stormer) from comment #19) > Review of attachment 334017 [details] [review] [review]: > > Seems even more wrong, some of the colorimetry information is found in the > encoded bitstream and should supersede what the container say. In general, I'd have said we usually do things the other way around - if there's info in the container that conflicts with the video stream, we take the container information - framerate or PAR, for example.
Yes but that's a separate issue, bug #750882 (which I also have on my list, I know what has to be done now)
Part of this is bug #772181 (as this is not limited to prores)
Created attachment 336526 [details] [review] qtmux: Write 4 bytes of zeroes at the end of the sample description extensions This is working around some broken software.
Created attachment 336530 [details] [review] qtmux: Fix writing of the 'fiel' extension atom This was also wrong for JPEG2000. Also write it for all MOV files and JPEG2000, not only for ProRes.
Created attachment 336680 [details] [review] qtmux: Write 'clap' atom for ProRes It's required for ProRes to work with other software. It is also in the MP4 standard, but inventing values here seems a bit tricky for the general case and it does not really give any extra information.
Created attachment 336684 [details] [review] qt: Add support for ProRes 4444 XQ And also 4444 in the muxer.
Created attachment 336685 [details] [review] qtmux: Set compressor name, horizontal/vertical resolution and depth for ProRes This is also required by some software to handle ProRes files.
Created attachment 336687 [details] [review] avcodecmap: Add variant to the ProRes caps
Attachment 336464 [details] pushed as fe38414 - qtmux: Write additional atoms for prores video Attachment 336465 [details] pushed as 4cff509 - atoms: 'pasp' atom is also part of MP4, write it always Attachment 336526 [details] pushed as b815c41 - qtmux: Write 4 bytes of zeroes at the end of the sample description extensions Attachment 336530 [details] pushed as 0584a71 - qtmux: Fix writing of the 'fiel' extension atom Attachment 336680 [details] pushed as a2c6921 - qtmux: Write 'clap' atom for ProRes Attachment 336684 [details] pushed as 7b56547 - qt: Add support for ProRes 4444 XQ Attachment 336685 [details] pushed as cba6cc4 - qtmux: Set compressor name, horizontal/vertical resolution and depth for ProRes
Attachment 336687 [details] pushed as 7f39f69 - avcodecmap: Add variant to the ProRes caps