diff mbox

[FFmpeg-devel] movenc: support GPMF track (gpmd) remuxing

Message ID 20170721083019.3174-1-u@pkh.me
State New
Headers show

Commit Message

Clément Bœsch July 21, 2017, 8:30 a.m. UTC 
From: Clément Bœsch <cboesch@gopro.com>

See https://github.com/gopro/gpmf-parser for more information on the
data stream itself.
---
 libavformat/movenc.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Michael Niedermayer July 21, 2017, 1:18 p.m. UTC | #1 
On Fri, Jul 21, 2017 at 10:30:19AM +0200, Clément Bœsch wrote:
> From: Clément Bœsch <cboesch@gopro.com>
> 
> See https://github.com/gopro/gpmf-parser for more information on the
> data stream itself.
> ---
>  libavformat/movenc.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)

can you add a fate test for this ?

thx

[...]
Derek Buitenhuis July 21, 2017, 1:21 p.m. UTC | #2 
On 7/21/2017 9:30 AM, Clément Bœsch wrote:
> +    avio_wb16(pb, 1); /* Data-reference index */

Why is this whole atom hardcoded (i.e. with is '1').

- Derek
Clément Bœsch July 21, 2017, 1:55 p.m. UTC | #3 
On Fri, Jul 21, 2017 at 02:21:23PM +0100, Derek Buitenhuis wrote:
> On 7/21/2017 9:30 AM, Clément Bœsch wrote:
> > +    avio_wb16(pb, 1); /* Data-reference index */
> 
> Why is this whole atom hardcoded (i.e. with is '1').
> 

It's basically the same layout for every stsd sample description. Check
the other tags that extend that layout, the also have that data ref idx to
1. I think I copied from the rtp code, but it's the same for the other as
well. This gpmd atom just extends the stsd sample description with a
reserved 32-bit value.
Derek Buitenhuis July 21, 2017, 2:03 p.m. UTC | #4 
On 7/21/2017 2:55 PM, Clément Bœsch wrote:
> It's basically the same layout for every stsd sample description. Check
> the other tags that extend that layout, the also have that data ref idx to
> 1. I think I copied from the rtp code, but it's the same for the other as
> well. This gpmd atom just extends the stsd sample description with a
> reserved 32-bit value.

How very useful.

- Derek
Clément Bœsch July 21, 2017, 2:09 p.m. UTC | #5 
On Fri, Jul 21, 2017 at 03:03:19PM +0100, Derek Buitenhuis wrote:
> On 7/21/2017 2:55 PM, Clément Bœsch wrote:
> > It's basically the same layout for every stsd sample description. Check
> > the other tags that extend that layout, the also have that data ref idx to
> > 1. I think I copied from the rtp code, but it's the same for the other as
> > well. This gpmd atom just extends the stsd sample description with a
> > reserved 32-bit value.
> 
> How very useful.
> 

Well, I didn't write the ISO specifications.

If you're refering to the reserved 32-bit value, I suppose it's meant to
allow extensions in the future. Typically it could be reused as a version
or version+flags, which would define more fields, who knows. It's probable
the presence of that field is necessary for some players, so it had to be
present in the first place.
Clément Bœsch July 21, 2017, 2:11 p.m. UTC | #6 
On Fri, Jul 21, 2017 at 04:09:29PM +0200, Clément Bœsch wrote:
> On Fri, Jul 21, 2017 at 03:03:19PM +0100, Derek Buitenhuis wrote:
> > On 7/21/2017 2:55 PM, Clément Bœsch wrote:
> > > It's basically the same layout for every stsd sample description. Check
> > > the other tags that extend that layout, the also have that data ref idx to
> > > 1. I think I copied from the rtp code, but it's the same for the other as
> > > well. This gpmd atom just extends the stsd sample description with a
> > > reserved 32-bit value.
> > 
> > How very useful.
> > 
> 
> Well, I didn't write the ISO specifications.
> 
> If you're refering to the reserved 32-bit value, I suppose it's meant to
> allow extensions in the future. Typically it could be reused as a version
> or version+flags, which would define more fields, who knows. It's probable

> the presence of that field is necessary for some players, so it had to be
                       ^^^^^
                  that atom (as in, the sample description)

> present in the first place.
>
Clément Bœsch July 24, 2017, 12:44 p.m. UTC | #7 
On Fri, Jul 21, 2017 at 03:18:26PM +0200, Michael Niedermayer wrote:
> On Fri, Jul 21, 2017 at 10:30:19AM +0200, Clément Bœsch wrote:
> > From: Clément Bœsch <cboesch@gopro.com>
> > 
> > See https://github.com/gopro/gpmf-parser for more information on the
> > data stream itself.
> > ---
> >  libavformat/movenc.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> 
> can you add a fate test for this ?
> 

added a test and pushed

thanks for the upload of the sample
diff mbox

Patch

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 3989ac167e..0e5b45d150 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -2060,6 +2060,18 @@  static int mov_write_tmcd_tag(AVIOContext *pb, MOVTrack *track)
     return update_size(pb, pos);
 }
 
+static int mov_write_gpmd_tag(AVIOContext *pb, const MOVTrack *track)
+{
+    int64_t pos = avio_tell(pb);
+    avio_wb32(pb, 0); /* size */
+    ffio_wfourcc(pb, "gpmd");
+    avio_wb32(pb, 0); /* Reserved */
+    avio_wb16(pb, 0); /* Reserved */
+    avio_wb16(pb, 1); /* Data-reference index */
+    avio_wb32(pb, 0); /* Reserved */
+    return update_size(pb, pos);
+}
+
 static int mov_write_stsd_tag(AVFormatContext *s, AVIOContext *pb, MOVMuxContext *mov, MOVTrack *track)
 {
     int64_t pos = avio_tell(pb);
@@ -2077,6 +2089,8 @@  static int mov_write_stsd_tag(AVFormatContext *s, AVIOContext *pb, MOVMuxContext
         mov_write_rtp_tag(pb, track);
     else if (track->par->codec_tag == MKTAG('t','m','c','d'))
         mov_write_tmcd_tag(pb, track);
+    else if (track->par->codec_tag == MKTAG('g','p','m','d'))
+        mov_write_gpmd_tag(pb, track);
     return update_size(pb, pos);
 }
 
@@ -2377,6 +2391,12 @@  static int mov_write_gmhd_tag(AVIOContext *pb, MOVTrack *track)
         ffio_wfourcc(pb, "tmcd");
         mov_write_tcmi_tag(pb, track);
         update_size(pb, tmcd_pos);
+    } else if (track->par->codec_tag == MKTAG('g','p','m','d')) {
+        int64_t gpmd_pos = avio_tell(pb);
+        avio_wb32(pb, 0); /* size */
+        ffio_wfourcc(pb, "gpmd");
+        avio_wb32(pb, 0); /* version */
+        update_size(pb, gpmd_pos);
     }
     return update_size(pb, pos);
 }
@@ -2443,6 +2463,9 @@  static int mov_write_hdlr_tag(AVFormatContext *s, AVIOContext *pb, MOVTrack *tra
         } else if (track->par->codec_tag == MKTAG('t','m','c','d')) {
             hdlr_type = "tmcd";
             descr = "TimeCodeHandler";
+        } else if (track->par->codec_tag == MKTAG('g','p','m','d')) {
+            hdlr_type = "meta";
+            descr = "GoPro MET"; // GoPro Metadata
         } else {
             av_log(s, AV_LOG_WARNING,
                    "Unknown hldr_type for %s, writing dummy values\n",
@@ -2514,6 +2537,8 @@  static int mov_write_minf_tag(AVFormatContext *s, AVIOContext *pb, MOVMuxContext
             mov_write_nmhd_tag(pb);
         else
             mov_write_gmhd_tag(pb, track);
+    } else if (track->tag == MKTAG('g','p','m','d')) {
+        mov_write_gmhd_tag(pb, track);
     }
     if (track->mode == MODE_MOV) /* FIXME: Why do it for MODE_MOV only ? */
         mov_write_hdlr_tag(s, pb, NULL);