Message ID | 20201117211126.402607-1-tfoucu@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [FFmpeg-devel] libavformat/mov.c: export vendor id as metadata | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | fail | Make fate failed |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | warning | Make fate failed |
On 18-11-2020 02:41 am, Thierry Foucu wrote: > --- > libavformat/mov.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index 2b90e31170..1f9163d658 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -2095,6 +2095,8 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb, > uint8_t codec_name[32] = { 0 }; > int64_t stsd_start; > unsigned int len; > + int32_t id = 0; > + char vendor_id[5]; > > /* The first 16 bytes of the video sample description are already > * read in ff_mov_read_stsd_entries() */ > @@ -2102,7 +2104,15 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb, > > avio_rb16(pb); /* version */ > avio_rb16(pb); /* revision level */ > - avio_rb32(pb); /* vendor */ > + id = avio_rb32(pb); /* vendor */ > + if (id != 0) { > + vendor_id[0] = (id >> 24) & 0xff; > + vendor_id[1] = (id >> 16) & 0xff; > + vendor_id[2] = (id >> 8) & 0xff; > + vendor_id[3] = (id >> 0) & 0xff; > + vendor_id[4] = 0; > + av_dict_set(&st->metadata, "vendor_id", vendor_id, 0); > + } > avio_rb32(pb); /* temporal quality */ > avio_rb32(pb); /* spatial quality */ > > @@ -2150,10 +2160,20 @@ static void mov_parse_stsd_audio(MOVContext *c, AVIOContext *pb, > { > int bits_per_sample, flags; > uint16_t version = avio_rb16(pb); > + int32_t id = 0; > + char vendor_id[5]; > AVDictionaryEntry *compatible_brands = av_dict_get(c->fc->metadata, "compatible_brands", NULL, AV_DICT_MATCH_CASE); > > avio_rb16(pb); /* revision level */ > - avio_rb32(pb); /* vendor */ > + id = avio_rb32(pb); /* vendor */ > + if (id != 0) { > + vendor_id[0] = (id >> 24) & 0xff; > + vendor_id[1] = (id >> 16) & 0xff; > + vendor_id[2] = (id >> 8) & 0xff; > + vendor_id[3] = (id >> 0) & 0xff; > + vendor_id[4] = 0; > + av_dict_set(&st->metadata, "vendor_id", vendor_id, 0); > + } Seems fine. But you likely have to update many FATE refs. FATE fails. See https://patchwork.ffmpeg.org/check/20508/ This automated check stops with the first failure. So check for all tests with '-k' Regards, Gyan
On Tue, Nov 17, 2020 at 9:54 PM Gyan Doshi <ffmpeg@gyani.pro> wrote: > > > On 18-11-2020 02:41 am, Thierry Foucu wrote: > > --- > > libavformat/mov.c | 24 ++++++++++++++++++++++-- > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c > > index 2b90e31170..1f9163d658 100644 > > --- a/libavformat/mov.c > > +++ b/libavformat/mov.c > > @@ -2095,6 +2095,8 @@ static void mov_parse_stsd_video(MOVContext *c, > AVIOContext *pb, > > uint8_t codec_name[32] = { 0 }; > > int64_t stsd_start; > > unsigned int len; > > + int32_t id = 0; > > + char vendor_id[5]; > > > > /* The first 16 bytes of the video sample description are already > > * read in ff_mov_read_stsd_entries() */ > > @@ -2102,7 +2104,15 @@ static void mov_parse_stsd_video(MOVContext *c, > AVIOContext *pb, > > > > avio_rb16(pb); /* version */ > > avio_rb16(pb); /* revision level */ > > - avio_rb32(pb); /* vendor */ > > + id = avio_rb32(pb); /* vendor */ > > + if (id != 0) { > > + vendor_id[0] = (id >> 24) & 0xff; > > + vendor_id[1] = (id >> 16) & 0xff; > > + vendor_id[2] = (id >> 8) & 0xff; > > + vendor_id[3] = (id >> 0) & 0xff; > > + vendor_id[4] = 0; > > + av_dict_set(&st->metadata, "vendor_id", vendor_id, 0); > > + } > > avio_rb32(pb); /* temporal quality */ > > avio_rb32(pb); /* spatial quality */ > > > > @@ -2150,10 +2160,20 @@ static void mov_parse_stsd_audio(MOVContext *c, > AVIOContext *pb, > > { > > int bits_per_sample, flags; > > uint16_t version = avio_rb16(pb); > > + int32_t id = 0; > > + char vendor_id[5]; > > AVDictionaryEntry *compatible_brands = > av_dict_get(c->fc->metadata, "compatible_brands", NULL, AV_DICT_MATCH_CASE); > > > > avio_rb16(pb); /* revision level */ > > - avio_rb32(pb); /* vendor */ > > + id = avio_rb32(pb); /* vendor */ > > + if (id != 0) { > > + vendor_id[0] = (id >> 24) & 0xff; > > + vendor_id[1] = (id >> 16) & 0xff; > > + vendor_id[2] = (id >> 8) & 0xff; > > + vendor_id[3] = (id >> 0) & 0xff; > > + vendor_id[4] = 0; > > + av_dict_set(&st->metadata, "vendor_id", vendor_id, 0); > > + } > > Seems fine. > > But you likely have to update many FATE refs. FATE fails. See > https://patchwork.ffmpeg.org/check/20508/ > This automated check stops with the first failure. So check for all > tests with '-k' > Thanks. I did look at the error but it does not seem related to this patch. Looking at it, i'm not sure how adding vendor ID could break a RGB24 in matroska. --- ./tests/ref/fate/rgb24-mkv 2020-10-29 20:05:36.014929264 +0000 +++ tests/data/fate/rgb24-mkv 2020-11-17 21:56:50.624249364 +0000 @@ -1,5 +1,5 @@ -fde8903c4df0ba8235dafcfd8a2f368c *tests/data/fate/rgb24-mkv.matroska -58216 tests/data/fate/rgb24-mkv.matroska +6244b8750d4155d3c9357bab26396ef9 *tests/data/fate/rgb24-mkv.matroska +58245 tests/data/fate/rgb24-mkv.matroska #tb 0: 1/10 #media_type 0: video #codec_id 0: rawvideo Test rgb24-mkv failed. Look at tests/data/fate/rgb24-mkv.err for details. tests/Makefile:255: recipe for target 'fate-rgb24-mkv' failed I will run fate and double check > Regards, > Gyan > _______________________________________________ > 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 18-11-2020 12:16 pm, Thierry Foucu wrote: > On Tue, Nov 17, 2020 at 9:54 PM Gyan Doshi <ffmpeg@gyani.pro> wrote: > >> >> On 18-11-2020 02:41 am, Thierry Foucu wrote: >>> --- >>> libavformat/mov.c | 24 ++++++++++++++++++++++-- >>> 1 file changed, 22 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavformat/mov.c b/libavformat/mov.c >>> index 2b90e31170..1f9163d658 100644 >>> --- a/libavformat/mov.c >>> +++ b/libavformat/mov.c >>> @@ -2095,6 +2095,8 @@ static void mov_parse_stsd_video(MOVContext *c, >> AVIOContext *pb, >>> uint8_t codec_name[32] = { 0 }; >>> int64_t stsd_start; >>> unsigned int len; >>> + int32_t id = 0; >>> + char vendor_id[5]; >>> >>> /* The first 16 bytes of the video sample description are already >>> * read in ff_mov_read_stsd_entries() */ >>> @@ -2102,7 +2104,15 @@ static void mov_parse_stsd_video(MOVContext *c, >> AVIOContext *pb, >>> avio_rb16(pb); /* version */ >>> avio_rb16(pb); /* revision level */ >>> - avio_rb32(pb); /* vendor */ >>> + id = avio_rb32(pb); /* vendor */ >>> + if (id != 0) { >>> + vendor_id[0] = (id >> 24) & 0xff; >>> + vendor_id[1] = (id >> 16) & 0xff; >>> + vendor_id[2] = (id >> 8) & 0xff; >>> + vendor_id[3] = (id >> 0) & 0xff; >>> + vendor_id[4] = 0; >>> + av_dict_set(&st->metadata, "vendor_id", vendor_id, 0); >>> + } >>> avio_rb32(pb); /* temporal quality */ >>> avio_rb32(pb); /* spatial quality */ >>> >>> @@ -2150,10 +2160,20 @@ static void mov_parse_stsd_audio(MOVContext *c, >> AVIOContext *pb, >>> { >>> int bits_per_sample, flags; >>> uint16_t version = avio_rb16(pb); >>> + int32_t id = 0; >>> + char vendor_id[5]; >>> AVDictionaryEntry *compatible_brands = >> av_dict_get(c->fc->metadata, "compatible_brands", NULL, AV_DICT_MATCH_CASE); >>> avio_rb16(pb); /* revision level */ >>> - avio_rb32(pb); /* vendor */ >>> + id = avio_rb32(pb); /* vendor */ >>> + if (id != 0) { >>> + vendor_id[0] = (id >> 24) & 0xff; >>> + vendor_id[1] = (id >> 16) & 0xff; >>> + vendor_id[2] = (id >> 8) & 0xff; >>> + vendor_id[3] = (id >> 0) & 0xff; >>> + vendor_id[4] = 0; >>> + av_dict_set(&st->metadata, "vendor_id", vendor_id, 0); >>> + } >> Seems fine. >> >> But you likely have to update many FATE refs. FATE fails. See >> https://patchwork.ffmpeg.org/check/20508/ >> This automated check stops with the first failure. So check for all >> tests with '-k' >> > Thanks. > I did look at the error but it does not seem related to this patch. > Looking at it, i'm not sure how adding vendor ID could break a RGB24 in > matroska. The test demuxes a MOV and transcodes streams to MKV. Since your patch adds a metadata entry, that changes the metadata written to the output. Gyan
On Wed, Nov 18, 2020 at 12:30 AM Gyan Doshi <ffmpeg@gyani.pro> wrote: > > > On 18-11-2020 12:16 pm, Thierry Foucu wrote: > > On Tue, Nov 17, 2020 at 9:54 PM Gyan Doshi <ffmpeg@gyani.pro> wrote: > > > >> > >> On 18-11-2020 02:41 am, Thierry Foucu wrote: > >>> --- > >>> libavformat/mov.c | 24 ++++++++++++++++++++++-- > >>> 1 file changed, 22 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/libavformat/mov.c b/libavformat/mov.c > >>> index 2b90e31170..1f9163d658 100644 > >>> --- a/libavformat/mov.c > >>> +++ b/libavformat/mov.c > >>> @@ -2095,6 +2095,8 @@ static void mov_parse_stsd_video(MOVContext *c, > >> AVIOContext *pb, > >>> uint8_t codec_name[32] = { 0 }; > >>> int64_t stsd_start; > >>> unsigned int len; > >>> + int32_t id = 0; > >>> + char vendor_id[5]; > >>> > >>> /* The first 16 bytes of the video sample description are > already > >>> * read in ff_mov_read_stsd_entries() */ > >>> @@ -2102,7 +2104,15 @@ static void mov_parse_stsd_video(MOVContext *c, > >> AVIOContext *pb, > >>> avio_rb16(pb); /* version */ > >>> avio_rb16(pb); /* revision level */ > >>> - avio_rb32(pb); /* vendor */ > >>> + id = avio_rb32(pb); /* vendor */ > >>> + if (id != 0) { > >>> + vendor_id[0] = (id >> 24) & 0xff; > >>> + vendor_id[1] = (id >> 16) & 0xff; > >>> + vendor_id[2] = (id >> 8) & 0xff; > >>> + vendor_id[3] = (id >> 0) & 0xff; > >>> + vendor_id[4] = 0; > >>> + av_dict_set(&st->metadata, "vendor_id", vendor_id, 0); > >>> + } > >>> avio_rb32(pb); /* temporal quality */ > >>> avio_rb32(pb); /* spatial quality */ > >>> > >>> @@ -2150,10 +2160,20 @@ static void mov_parse_stsd_audio(MOVContext *c, > >> AVIOContext *pb, > >>> { > >>> int bits_per_sample, flags; > >>> uint16_t version = avio_rb16(pb); > >>> + int32_t id = 0; > >>> + char vendor_id[5]; > >>> AVDictionaryEntry *compatible_brands = > >> av_dict_get(c->fc->metadata, "compatible_brands", NULL, > AV_DICT_MATCH_CASE); > >>> avio_rb16(pb); /* revision level */ > >>> - avio_rb32(pb); /* vendor */ > >>> + id = avio_rb32(pb); /* vendor */ > >>> + if (id != 0) { > >>> + vendor_id[0] = (id >> 24) & 0xff; > >>> + vendor_id[1] = (id >> 16) & 0xff; > >>> + vendor_id[2] = (id >> 8) & 0xff; > >>> + vendor_id[3] = (id >> 0) & 0xff; > >>> + vendor_id[4] = 0; > >>> + av_dict_set(&st->metadata, "vendor_id", vendor_id, 0); > >>> + } > >> Seems fine. > >> > >> But you likely have to update many FATE refs. FATE fails. See > >> https://patchwork.ffmpeg.org/check/20508/ > >> This automated check stops with the first failure. So check for all > >> tests with '-k' > >> > > Thanks. > > I did look at the error but it does not seem related to this patch. > > Looking at it, i'm not sure how adding vendor ID could break a RGB24 in > > matroska. > > The test demuxes a MOV and transcodes streams to MKV. Since your patch > adds a metadata entry, that changes the metadata written to the output. > > Ah.. I see.. Will be sending new patch soon with fate tests fixed > Gyan > _______________________________________________ > 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".
diff --git a/libavformat/mov.c b/libavformat/mov.c index 2b90e31170..1f9163d658 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -2095,6 +2095,8 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb, uint8_t codec_name[32] = { 0 }; int64_t stsd_start; unsigned int len; + int32_t id = 0; + char vendor_id[5]; /* The first 16 bytes of the video sample description are already * read in ff_mov_read_stsd_entries() */ @@ -2102,7 +2104,15 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb, avio_rb16(pb); /* version */ avio_rb16(pb); /* revision level */ - avio_rb32(pb); /* vendor */ + id = avio_rb32(pb); /* vendor */ + if (id != 0) { + vendor_id[0] = (id >> 24) & 0xff; + vendor_id[1] = (id >> 16) & 0xff; + vendor_id[2] = (id >> 8) & 0xff; + vendor_id[3] = (id >> 0) & 0xff; + vendor_id[4] = 0; + av_dict_set(&st->metadata, "vendor_id", vendor_id, 0); + } avio_rb32(pb); /* temporal quality */ avio_rb32(pb); /* spatial quality */ @@ -2150,10 +2160,20 @@ static void mov_parse_stsd_audio(MOVContext *c, AVIOContext *pb, { int bits_per_sample, flags; uint16_t version = avio_rb16(pb); + int32_t id = 0; + char vendor_id[5]; AVDictionaryEntry *compatible_brands = av_dict_get(c->fc->metadata, "compatible_brands", NULL, AV_DICT_MATCH_CASE); avio_rb16(pb); /* revision level */ - avio_rb32(pb); /* vendor */ + id = avio_rb32(pb); /* vendor */ + if (id != 0) { + vendor_id[0] = (id >> 24) & 0xff; + vendor_id[1] = (id >> 16) & 0xff; + vendor_id[2] = (id >> 8) & 0xff; + vendor_id[3] = (id >> 0) & 0xff; + vendor_id[4] = 0; + av_dict_set(&st->metadata, "vendor_id", vendor_id, 0); + } st->codecpar->channels = avio_rb16(pb); /* channel count */ st->codecpar->bits_per_coded_sample = avio_rb16(pb); /* sample size */