[FFmpeg-devel,1/3] avformat/movenc: write the major brand also as the first compatible brand

Submitted by James Almer on Dec. 2, 2019, 4:18 p.m.

Details

Message ID 20191202161838.1746-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Dec. 2, 2019, 4:18 p.m.
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(-)

Comments

Michael Niedermayer Dec. 3, 2019, 8:19 a.m.
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

[...]
Carl Eugen Hoyos Dec. 3, 2019, 8:20 a.m.
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
James Almer Dec. 3, 2019, 12:10 p.m.
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.
James Almer Dec. 3, 2019, 8 p.m.
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".
>

Patch hide | download patch | download mbox

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)