| Message ID | 1498664522-44645-2-git-send-email-derek.buitenhuis@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jun 28, 2017 at 04:41:58PM +0100, Derek Buitenhuis wrote: > From: John Stebbins <stebbins@jetheaddev.com> > > ff_mp4_obj_type contains the wrong type of tags for > AVOutputFormat.codec_tag. AVOutputFormat.codec_tag is used to > validate AVCodecParameters.codec_tag so needs to be the same > type of tag. > > Creates new tag lists for mp4 and ismv. New tag lists support > same list of codecs found in ff_mp4_obj_type. psp uses the same > tag list as mp4 since these both use mp4_get_codec_tag to look up tags. > > (cherry picked from commit 713efb2c0d013a42be4051adb7cd90a7c2cbbb4f) > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > libavformat/movenc.c | 42 +++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 39 insertions(+), 3 deletions(-) Fails to build src/libavformat/movenc.c:6450:7: error: ‘AV_CODEC_ID_EVR’ undeclared here (not in a function) { AV_CODEC_ID_EVR , MKTAG('m', 'p', '4', 'v') }, ^ make: *** [libavformat/movenc.o] Error 1 [...]
On 6/29/2017 2:03 AM, Michael Niedermayer wrote: > Fails to build > > src/libavformat/movenc.c:6450:7: error: ‘AV_CODEC_ID_EVR’ undeclared here (not in a function) > { AV_CODEC_ID_EVR , MKTAG('m', 'p', '4', 'v') }, Looks like I forgot to regenerate the .patch files after I fixed that and ran FATE. Woops. This was already amended locally. I'll send that. - Derek
On 6/28/2017 12:41 PM, Derek Buitenhuis wrote: > From: John Stebbins <stebbins@jetheaddev.com> > > ff_mp4_obj_type contains the wrong type of tags for > AVOutputFormat.codec_tag. AVOutputFormat.codec_tag is used to > validate AVCodecParameters.codec_tag so needs to be the same > type of tag. > > Creates new tag lists for mp4 and ismv. New tag lists support > same list of codecs found in ff_mp4_obj_type. psp uses the same > tag list as mp4 since these both use mp4_get_codec_tag to look up tags. > > (cherry picked from commit 713efb2c0d013a42be4051adb7cd90a7c2cbbb4f) > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > libavformat/movenc.c | 42 +++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 39 insertions(+), 3 deletions(-) > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c > index ca389e3..2a07e00 100644 > --- a/libavformat/movenc.c > +++ b/libavformat/movenc.c > @@ -6488,6 +6488,41 @@ static int mov_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt) > return ret; > } > > +const AVCodecTag codec_mp4_tags[] = { > + { AV_CODEC_ID_MPEG4 , MKTAG('m', 'p', '4', 'v') }, > + { AV_CODEC_ID_H264 , MKTAG('a', 'v', 'c', '1') }, > + { AV_CODEC_ID_HEVC , MKTAG('h', 'e', 'v', '1') }, > + { AV_CODEC_ID_MPEG2VIDEO , MKTAG('m', 'p', '4', 'v') }, > + { AV_CODEC_ID_MPEG1VIDEO , MKTAG('m', 'p', '4', 'v') }, > + { AV_CODEC_ID_MJPEG , MKTAG('m', 'p', '4', 'v') }, > + { AV_CODEC_ID_PNG , MKTAG('m', 'p', '4', 'v') }, > + { AV_CODEC_ID_JPEG2000 , MKTAG('m', 'p', '4', 'v') }, > + { AV_CODEC_ID_VC1 , MKTAG('v', 'c', '-', '1') }, > + { AV_CODEC_ID_DIRAC , MKTAG('d', 'r', 'a', 'c') }, > + { AV_CODEC_ID_TSCC2 , MKTAG('m', 'p', '4', 'v') }, > + { AV_CODEC_ID_VP9 , MKTAG('v', 'p', '0', '9') }, > + { AV_CODEC_ID_EVR , MKTAG('m', 'p', '4', 'v') }, > + { AV_CODEC_ID_AAC , MKTAG('m', 'p', '4', 'a') }, > + { AV_CODEC_ID_MP4ALS , MKTAG('m', 'p', '4', 'a') }, > + { AV_CODEC_ID_MP3 , MKTAG('m', 'p', '4', 'a') }, > + { AV_CODEC_ID_MP2 , MKTAG('m', 'p', '4', 'a') }, > + { AV_CODEC_ID_AC3 , MKTAG('a', 'c', '-', '3') }, > + { AV_CODEC_ID_EAC3 , MKTAG('a', 'c', '-', '3') }, Should be ec-3. Changing it fixes fate-copy-trac3074 as pointed by Michael in a reply to patch 2/5. > + { AV_CODEC_ID_DTS , MKTAG('m', 'p', '4', 'a') }, Doesn't DTS have a bunch of unique tags? The ones listed in ff_codec_movaudio_tags and http://www.mp4ra.org/codecs.html > + { AV_CODEC_ID_FLAC , MKTAG('f', 'L', 'a', 'C') }, > + { AV_CODEC_ID_OPUS , MKTAG('O', 'p', 'u', 's') }, > + { AV_CODEC_ID_VORBIS , MKTAG('m', 'p', '4', 'a') }, > + { AV_CODEC_ID_QCELP , MKTAG('m', 'p', '4', 'a') }, > + { AV_CODEC_ID_DVD_SUBTITLE, MKTAG('m', 'p', '4', 's') }, > + { AV_CODEC_ID_MOV_TEXT , MKTAG('t', 'x', '3', 'g') }, > + { AV_CODEC_ID_NONE , 0 }, > +}; > + > +const AVCodecTag codec_ism_tags[] = { > + { AV_CODEC_ID_WMAPRO , MKTAG('w', 'm', 'a', ' ') }, > + { AV_CODEC_ID_NONE , 0 }, > +}; > + > #if CONFIG_MOV_MUXER > MOV_CLASS(mov) > AVOutputFormat ff_mov_muxer = { > @@ -6548,7 +6583,7 @@ AVOutputFormat ff_mp4_muxer = { > .write_trailer = mov_write_trailer, > .deinit = mov_free, > .flags = AVFMT_GLOBALHEADER | AVFMT_ALLOW_FLUSH | AVFMT_TS_NEGATIVE, > - .codec_tag = (const AVCodecTag* const []){ ff_mp4_obj_type, 0 }, > + .codec_tag = (const AVCodecTag* const []){ codec_mp4_tags, 0 }, > .check_bitstream = mov_check_bitstream, > .priv_class = &mp4_muxer_class, > }; > @@ -6569,7 +6604,7 @@ AVOutputFormat ff_psp_muxer = { > .write_trailer = mov_write_trailer, > .deinit = mov_free, > .flags = AVFMT_GLOBALHEADER | AVFMT_ALLOW_FLUSH | AVFMT_TS_NEGATIVE, > - .codec_tag = (const AVCodecTag* const []){ ff_mp4_obj_type, 0 }, > + .codec_tag = (const AVCodecTag* const []){ codec_mp4_tags, 0 }, > .check_bitstream = mov_check_bitstream, > .priv_class = &psp_muxer_class, > }; > @@ -6631,7 +6666,8 @@ AVOutputFormat ff_ismv_muxer = { > .write_trailer = mov_write_trailer, > .deinit = mov_free, > .flags = AVFMT_GLOBALHEADER | AVFMT_ALLOW_FLUSH | AVFMT_TS_NEGATIVE, > - .codec_tag = (const AVCodecTag* const []){ ff_mp4_obj_type, 0 }, > + .codec_tag = (const AVCodecTag* const []){ > + codec_mp4_tags, codec_ism_tags, 0 }, > .check_bitstream = mov_check_bitstream, > .priv_class = &ismv_muxer_class, > }; >
On 7/3/2017 3:00 AM, James Almer wrote: >> + { AV_CODEC_ID_EAC3 , MKTAG('a', 'c', '-', '3') }, > Should be ec-3. Changing it fixes fate-copy-trac3074 as pointed by > Michael in a reply to patch 2/5. OK, this was my bug; Libav does't have that entry at all. > >> + { AV_CODEC_ID_DTS , MKTAG('m', 'p', '4', 'a') }, > Doesn't DTS have a bunch of unique tags? The ones listed in > ff_codec_movaudio_tags and http://www.mp4ra.org/codecs.html I'll check into it. - Derek
On 7/3/2017 3:00 AM, James Almer wrote: > Doesn't DTS have a bunch of unique tags? The ones listed in > ff_codec_movaudio_tags and http://www.mp4ra.org/codecs.html It looks like those only cover DTS extensions, which we all mark s AV_CODEC_ID_DTS. Is the DTS base layer 'mp4a'? I don't have a source on what is correct to do here, or how to fit it in (since this seems to imply DTS should have a 1-to-N mapping). Does copying these work post-patch? FATE passes, but we probably don't have stuff like a DTS-HD-MA-in-MP4 sample. (I changed the EAC3 entry locally but have not sent a new patch since I'm not 100% sure about the DTS stuff.) - Derek
On Tue, Jul 4, 2017 at 5:09 PM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > On 7/3/2017 3:00 AM, James Almer wrote: >> Doesn't DTS have a bunch of unique tags? The ones listed in >> ff_codec_movaudio_tags and http://www.mp4ra.org/codecs.html > > It looks like those only cover DTS extensions, which we all > mark s AV_CODEC_ID_DTS. Is the DTS base layer 'mp4a'? I don't > have a source on what is correct to do here, or how to fit it > in (since this seems to imply DTS should have a 1-to-N mapping). > > "dtsc" appears to be the base DTS codec, according to http://www.mp4ra.org/codecs.html "mp4a" only applies to codecs defined in the official MP4 specification. - Hendrik
On 7/4/2017 12:09 PM, Derek Buitenhuis wrote: > On 7/3/2017 3:00 AM, James Almer wrote: >> Doesn't DTS have a bunch of unique tags? The ones listed in >> ff_codec_movaudio_tags and http://www.mp4ra.org/codecs.html > > It looks like those only cover DTS extensions, which we all > mark s AV_CODEC_ID_DTS. Is the DTS base layer 'mp4a'? I don't > have a source on what is correct to do here, or how to fit it > in (since this seems to imply DTS should have a 1-to-N mapping). > > Does copying these work post-patch? FATE passes, but we probably > don't have stuff like a DTS-HD-MA-in-MP4 sample. > > (I changed the EAC3 entry locally but have not sent a new patch > since I'm not 100% sure about the DTS stuff.) Your patchset doesn't seem to affect DTS muxing. It's the same before and after. Before it would default to mp4a apparently for being audio and having an entry in obj_type. After your patchset, mp4a is explicitly set for DTS in codec_mp4_tags. According to mp4ra.org, DTS core should have a dtsc codec tag. The rest are for the extensions. A quick test changing mp4a to dtsc on top of your patchset resulted in a wildly different output file, among other reasons because the esds box isn't written (it expects an mp4a tag). The DTS in ISOMF spec doesn't seem to be publicly available after a quick search, so IMO just push this as is, which will retain the current behavior, at least until we're sure how it should be done.
On 7/4/2017 4:31 PM, Hendrik Leppkes wrote: > "dtsc" appears to be the base DTS codec, according to > http://www.mp4ra.org/codecs.html > "mp4a" only applies to codecs defined in the official MP4 specification. Oh, I missed that. Still not sure if we need to handle the extra layered stuff inside movenc, though; I'm not familiar enough with our DTS code. - Derek
On 7/4/2017 4:52 PM, James Almer wrote: > According to mp4ra.org, DTS core should have a dtsc codec tag. The rest > are for the extensions. A quick test changing mp4a to dtsc on top of > your patchset resulted in a wildly different output file, among other > reasons because the esds box isn't written (it expects an mp4a tag). > > The DTS in ISOMF spec doesn't seem to be publicly available after a > quick search, so IMO just push this as is, which will retain the current > behavior, at least until we're sure how it should be done. I guess 'unchanged output' is as good of a result as any, for now... - Derek
On 7/4/2017 12:57 PM, Derek Buitenhuis wrote: > On 7/4/2017 4:31 PM, Hendrik Leppkes wrote: >> "dtsc" appears to be the base DTS codec, according to >> http://www.mp4ra.org/codecs.html >> "mp4a" only applies to codecs defined in the official MP4 specification. > > Oh, I missed that. > > Still not sure if we need to handle the extra layered stuff inside movenc, > though; I'm not familiar enough with our DTS code. > > - Derek If you mean how to detect the DTS extensions, shouldn't checking codecpar->profile be enough for that?
On 7/4/2017 4:58 PM, Derek Buitenhuis wrote:
> I guess 'unchanged output' is as good of a result as any, for now...
Pushed patches 0.5 and 1.
Any comments on the other four?
- Derek
diff --git a/libavformat/movenc.c b/libavformat/movenc.c index ca389e3..2a07e00 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -6488,6 +6488,41 @@ static int mov_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt) return ret; } +const AVCodecTag codec_mp4_tags[] = { + { AV_CODEC_ID_MPEG4 , MKTAG('m', 'p', '4', 'v') }, + { AV_CODEC_ID_H264 , MKTAG('a', 'v', 'c', '1') }, + { AV_CODEC_ID_HEVC , MKTAG('h', 'e', 'v', '1') }, + { AV_CODEC_ID_MPEG2VIDEO , MKTAG('m', 'p', '4', 'v') }, + { AV_CODEC_ID_MPEG1VIDEO , MKTAG('m', 'p', '4', 'v') }, + { AV_CODEC_ID_MJPEG , MKTAG('m', 'p', '4', 'v') }, + { AV_CODEC_ID_PNG , MKTAG('m', 'p', '4', 'v') }, + { AV_CODEC_ID_JPEG2000 , MKTAG('m', 'p', '4', 'v') }, + { AV_CODEC_ID_VC1 , MKTAG('v', 'c', '-', '1') }, + { AV_CODEC_ID_DIRAC , MKTAG('d', 'r', 'a', 'c') }, + { AV_CODEC_ID_TSCC2 , MKTAG('m', 'p', '4', 'v') }, + { AV_CODEC_ID_VP9 , MKTAG('v', 'p', '0', '9') }, + { AV_CODEC_ID_EVR , MKTAG('m', 'p', '4', 'v') }, + { AV_CODEC_ID_AAC , MKTAG('m', 'p', '4', 'a') }, + { AV_CODEC_ID_MP4ALS , MKTAG('m', 'p', '4', 'a') }, + { AV_CODEC_ID_MP3 , MKTAG('m', 'p', '4', 'a') }, + { AV_CODEC_ID_MP2 , MKTAG('m', 'p', '4', 'a') }, + { AV_CODEC_ID_AC3 , MKTAG('a', 'c', '-', '3') }, + { AV_CODEC_ID_EAC3 , MKTAG('a', 'c', '-', '3') }, + { AV_CODEC_ID_DTS , MKTAG('m', 'p', '4', 'a') }, + { AV_CODEC_ID_FLAC , MKTAG('f', 'L', 'a', 'C') }, + { AV_CODEC_ID_OPUS , MKTAG('O', 'p', 'u', 's') }, + { AV_CODEC_ID_VORBIS , MKTAG('m', 'p', '4', 'a') }, + { AV_CODEC_ID_QCELP , MKTAG('m', 'p', '4', 'a') }, + { AV_CODEC_ID_DVD_SUBTITLE, MKTAG('m', 'p', '4', 's') }, + { AV_CODEC_ID_MOV_TEXT , MKTAG('t', 'x', '3', 'g') }, + { AV_CODEC_ID_NONE , 0 }, +}; + +const AVCodecTag codec_ism_tags[] = { + { AV_CODEC_ID_WMAPRO , MKTAG('w', 'm', 'a', ' ') }, + { AV_CODEC_ID_NONE , 0 }, +}; + #if CONFIG_MOV_MUXER MOV_CLASS(mov) AVOutputFormat ff_mov_muxer = { @@ -6548,7 +6583,7 @@ AVOutputFormat ff_mp4_muxer = { .write_trailer = mov_write_trailer, .deinit = mov_free, .flags = AVFMT_GLOBALHEADER | AVFMT_ALLOW_FLUSH | AVFMT_TS_NEGATIVE, - .codec_tag = (const AVCodecTag* const []){ ff_mp4_obj_type, 0 }, + .codec_tag = (const AVCodecTag* const []){ codec_mp4_tags, 0 }, .check_bitstream = mov_check_bitstream, .priv_class = &mp4_muxer_class, }; @@ -6569,7 +6604,7 @@ AVOutputFormat ff_psp_muxer = { .write_trailer = mov_write_trailer, .deinit = mov_free, .flags = AVFMT_GLOBALHEADER | AVFMT_ALLOW_FLUSH | AVFMT_TS_NEGATIVE, - .codec_tag = (const AVCodecTag* const []){ ff_mp4_obj_type, 0 }, + .codec_tag = (const AVCodecTag* const []){ codec_mp4_tags, 0 }, .check_bitstream = mov_check_bitstream, .priv_class = &psp_muxer_class, }; @@ -6631,7 +6666,8 @@ AVOutputFormat ff_ismv_muxer = { .write_trailer = mov_write_trailer, .deinit = mov_free, .flags = AVFMT_GLOBALHEADER | AVFMT_ALLOW_FLUSH | AVFMT_TS_NEGATIVE, - .codec_tag = (const AVCodecTag* const []){ ff_mp4_obj_type, 0 }, + .codec_tag = (const AVCodecTag* const []){ + codec_mp4_tags, codec_ism_tags, 0 }, .check_bitstream = mov_check_bitstream, .priv_class = &ismv_muxer_class, };