diff mbox series

[FFmpeg-devel] libavformat/mov.c: export vendor id as metadata

Message ID 20201117211126.402607-1-tfoucu@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel] libavformat/mov.c: export vendor id as metadata | expand

Checks

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

Commit Message

Thierry Foucu Nov. 17, 2020, 9:11 p.m. UTC 
---
 libavformat/mov.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

Comments

Gyan Doshi Nov. 18, 2020, 5:53 a.m. UTC | #1 
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
Thierry Foucu Nov. 18, 2020, 6:46 a.m. UTC | #2 
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".
Gyan Doshi Nov. 18, 2020, 8:30 a.m. UTC | #3 
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
Thierry Foucu Nov. 18, 2020, 6:23 p.m. UTC | #4 
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 mbox series

Patch

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 */