After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 777100 - qtmux: Write tapt atom for MOV files if PAR not 1/1
qtmux: Write tapt atom for MOV files if PAR not 1/1
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other All
: Normal normal
: 1.11.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-01-10 16:21 UTC by Sebastian Dröge (slomo)
Modified: 2017-01-12 17:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtmux: Write tapt atom for MOV files if PAR not 1/1 (7.14 KB, patch)
2017-01-10 16:21 UTC, Sebastian Dröge (slomo)
none Details | Review
qtmux: Write tapt atom for MOV files if PAR not 1/1 (8.34 KB, patch)
2017-01-11 10:25 UTC, Sebastian Dröge (slomo)
committed Details | Review
[PATCH] qtmux: Calculate clean aperture size (4.10 KB, patch)
2017-01-12 13:40 UTC, Georg Lippitsch
none Details | Review
[PATCH] qtmux: Calculate clean aperture size (4.11 KB, patch)
2017-01-12 16:25 UTC, Georg Lippitsch
committed Details | Review

Description Sebastian Dröge (slomo) 2017-01-10 16:21:27 UTC
See commit message
Comment 1 Sebastian Dröge (slomo) 2017-01-10 16:21:31 UTC
Created attachment 343253 [details] [review]
qtmux: Write tapt atom for MOV files if PAR not 1/1

Needed for QuickTime 7 to properly play files.

Also write the clap atom for MOV files always, not only when ProRes is
used as a video codec. It's mandatory for MOV.
Comment 2 Sebastian Dröge (slomo) 2017-01-11 10:11:34 UTC
Comment on attachment 343253 [details] [review]
qtmux: Write tapt atom for MOV files if PAR not 1/1

The tapt should be directly in the trak atom, not in the STE
Comment 3 Sebastian Dröge (slomo) 2017-01-11 10:25:43 UTC
Created attachment 343291 [details] [review]
qtmux: Write tapt atom for MOV files if PAR not 1/1

Needed for QuickTime 7 to properly play files.

Also write the clap atom for MOV files always, not only when ProRes is
used as a video codec. It's mandatory for MOV.
Comment 4 Georg Lippitsch 2017-01-12 13:40:24 UTC
Created attachment 343359 [details] [review]
[PATCH] qtmux: Calculate clean aperture size

A more generic way to calculate clef and clap values.
This is still based on a guess of display aspect ratio (either 16:9 or 4:3), but should be correct for more combinations of size and PAR than the old patch.
Comment 5 Sebastian Dröge (slomo) 2017-01-12 15:33:09 UTC
Review of attachment 343359 [details] [review]:

Generally looks good, just two minor things

::: gst/isomp4/gstqtmux.c
@@ +4418,2 @@
+  if (qtmux_klass->format == GST_QT_MUX_FORMAT_QT &&
+      width > 640 && width <= 1052 && height >= 480 && height <= 576) {

Why only for <HD resolutions? Shouldn't at least clap also be written for HD?

@@ +4445,3 @@
+          height * par_den);
+      dar_num = width * par_num / cdiv;
+      dar_den = height * par_den / cdiv;

You calculate width*par_num (and the other one) twice here
Comment 6 Georg Lippitsch 2017-01-12 16:07:35 UTC
'clap' should definitely be ommited for HD, and 'tapt' also doesn't really make sense there.
AFAIK, all these things only makes sense for data derived from analogue TV signals in the SD PAL/NTSC world.
Comment 7 Georg Lippitsch 2017-01-12 16:25:42 UTC
Created attachment 343379 [details] [review]
[PATCH] qtmux: Calculate clean aperture size
Comment 8 Sebastian Dröge (slomo) 2017-01-12 17:09:48 UTC
commit fcdba653021dac7acdc017e5811d2b64e7b995cf
Author: Georg Lippitsch <glippitsch@toolsonair.com>
Date:   Wed Jan 11 18:29:05 2017 +0100

    qtmux: Calculate clean aperture size
    
    Calculate clean aperture dimensions by first guessing
    display aspect ratio based on pixel aspect ratio and
    frame size.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=777100

commit e12c94065d013986a5f69a14253ee9c439e41301
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Jan 10 18:19:55 2017 +0200

    qtmux: Write tapt atom for MOV files if PAR not 1/1
    
    Needed for QuickTime 7 to properly play files.
    
    Also write the clap atom for MOV files always, not only when ProRes is
    used as a video codec. It's mandatory for MOV.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=777100