| Message ID | 20191202161838.1746-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Dec 02, 2019 at 01:18:36PM -0300, James Almer wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > This is meant to be squashed with patch 2/3, otherwise it will write duplicate > compatible brands, and a lot of tests will have to be updated twice. I dont think 2 updates are a problem if it keeps the commits more readable [...]
Am Mo., 2. Dez. 2019 um 17:19 Uhr schrieb James Almer <jamrial@gmail.com>: > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > This is meant to be squashed with patch 2/3, otherwise it will write duplicate > compatible brands, and a lot of tests will have to be updated twice. > > I'm sending it like this to make reviewing/reading easier. Could you add a line about why writing the major brand as compatible brand is a good idea? Is this recommended or does it fix a player? Thank you, Carl Eugen
On 12/3/2019 5:20 AM, Carl Eugen Hoyos wrote: > Am Mo., 2. Dez. 2019 um 17:19 Uhr schrieb James Almer <jamrial@gmail.com>: >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> This is meant to be squashed with patch 2/3, otherwise it will write duplicate >> compatible brands, and a lot of tests will have to be updated twice. >> >> I'm sending it like this to make reviewing/reading easier. > > Could you add a line about why writing the major brand as compatible > brand is a good idea? Is this recommended or does it fix a player? > > Thank you, Carl Eugen The DASH IF validator complains about it when the major isn't listed again as a compatible brand. We're already writing the major a second time in some specific cases. See the checks for mov, psp and 3gp I'm removing at the end of this patch. For mp4 it depended on a magic combination of options. I'm just making it cleaner and more general. There's also ticket 8375 which may or may not be fixed by this.
On 12/3/2019 5:19 AM, Michael Niedermayer wrote: > On Mon, Dec 02, 2019 at 01:18:36PM -0300, James Almer wrote: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- > >> This is meant to be squashed with patch 2/3, otherwise it will write duplicate >> compatible brands, and a lot of tests will have to be updated twice. > > I dont think 2 updates are a problem if it keeps the commits more readable Alright. > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On 12/3/2019 5:00 PM, James Almer wrote: > On 12/3/2019 5:19 AM, Michael Niedermayer wrote: >> On Mon, Dec 02, 2019 at 01:18:36PM -0300, James Almer wrote: >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >> >>> This is meant to be squashed with patch 2/3, otherwise it will write duplicate >>> compatible brands, and a lot of tests will have to be updated twice. >> >> I dont think 2 updates are a problem if it keeps the commits more readable > > Alright. Will apply this set soon without squashing.
On 12/9/2019 6:07 PM, James Almer wrote: > On 12/3/2019 5:00 PM, James Almer wrote: >> On 12/3/2019 5:19 AM, Michael Niedermayer wrote: >>> On Mon, Dec 02, 2019 at 01:18:36PM -0300, James Almer wrote: >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>> >>>> This is meant to be squashed with patch 2/3, otherwise it will write duplicate >>>> compatible brands, and a lot of tests will have to be updated twice. >>> >>> I dont think 2 updates are a problem if it keeps the commits more readable >> >> Alright. > > Will apply this set soon without squashing. Updated first patch with the required fate changes and pushed. Thanks.
diff --git a/libavformat/movenc.c b/libavformat/movenc.c index dd144ae20a..afce95042e 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -4740,27 +4740,11 @@ static int mov_write_mdat_tag(AVIOContext *pb, MOVMuxContext *mov) return 0; } -/* TODO: This needs to be more general */ -static int mov_write_ftyp_tag(AVIOContext *pb, AVFormatContext *s) +static void mov_write_ftyp_tag_internal(AVIOContext *pb, AVFormatContext *s, + int has_h264, int has_video, int write_minor) { MOVMuxContext *mov = s->priv_data; - int64_t pos = avio_tell(pb); - int has_h264 = 0, has_video = 0; int minor = 0x200; - int i; - - for (i = 0; i < s->nb_streams; i++) { - AVStream *st = s->streams[i]; - if (is_cover_image(st)) - continue; - if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) - has_video = 1; - if (st->codecpar->codec_id == AV_CODEC_ID_H264) - has_h264 = 1; - } - - avio_wb32(pb, 0); /* size */ - ffio_wfourcc(pb, "ftyp"); if (mov->major_brand && strlen(mov->major_brand) >= 4) ffio_wfourcc(pb, mov->major_brand); @@ -4787,11 +4771,36 @@ static int mov_write_ftyp_tag(AVIOContext *pb, AVFormatContext *s) else ffio_wfourcc(pb, "qt "); - avio_wb32(pb, minor); + if (write_minor) + avio_wb32(pb, minor); +} - if (mov->mode == MODE_MOV) - ffio_wfourcc(pb, "qt "); - else if (mov->mode == MODE_ISM) { +static int mov_write_ftyp_tag(AVIOContext *pb, AVFormatContext *s) +{ + MOVMuxContext *mov = s->priv_data; + int64_t pos = avio_tell(pb); + int has_h264 = 0, has_video = 0; + int i; + + for (i = 0; i < s->nb_streams; i++) { + AVStream *st = s->streams[i]; + if (is_cover_image(st)) + continue; + if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) + has_video = 1; + if (st->codecpar->codec_id == AV_CODEC_ID_H264) + has_h264 = 1; + } + + avio_wb32(pb, 0); /* size */ + ffio_wfourcc(pb, "ftyp"); + + // Write major brand + mov_write_ftyp_tag_internal(pb, s, has_h264, has_video, 1); + // Write the major brand as the first compatible brand as well + mov_write_ftyp_tag_internal(pb, s, has_h264, has_video, 0); + + if (mov->mode == MODE_ISM) { ffio_wfourcc(pb, "piff"); } else if (!(mov->flags & FF_MOV_FLAG_DEFAULT_BASE_MOOF)) { ffio_wfourcc(pb, "isom"); @@ -4805,13 +4814,7 @@ static int mov_write_ftyp_tag(AVIOContext *pb, AVFormatContext *s) if (mov->flags & FF_MOV_FLAG_FRAGMENT && mov->mode != MODE_ISM) ffio_wfourcc(pb, "iso6"); - if (mov->mode == MODE_3GP) - ffio_wfourcc(pb, has_h264 ? "3gp6":"3gp4"); - else if (mov->mode & MODE_3G2) - ffio_wfourcc(pb, has_h264 ? "3g2b":"3g2a"); - else if (mov->mode == MODE_PSP) - ffio_wfourcc(pb, "MSNV"); - else if (mov->mode == MODE_MP4) + if (mov->mode == MODE_MP4) ffio_wfourcc(pb, "mp41"); if (mov->flags & FF_MOV_FLAG_DASH && mov->flags & FF_MOV_FLAG_GLOBAL_SIDX)
Signed-off-by: James Almer <jamrial@gmail.com> --- This is meant to be squashed with patch 2/3, otherwise it will write duplicate compatible brands, and a lot of tests will have to be updated twice. I'm sending it like this to make reviewing/reading easier. libavformat/movenc.c | 61 +++++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 29 deletions(-)