[FFmpeg-devel,2/2] avformat/ivfenc: use the av1_metadata bsf to insert Temporal Delimiter OBUs if needed

Submitted by James Almer on Oct. 3, 2018, 12:18 a.m.

Details

Message ID 20181003001805.9312-2-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Oct. 3, 2018, 12:18 a.m.
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/ivfenc.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Mark Thompson Oct. 3, 2018, 11:01 p.m.
On 03/10/18 01:18, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavformat/ivfenc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c
> index 66441a2a43..adf72117e9 100644
> --- a/libavformat/ivfenc.c
> +++ b/libavformat/ivfenc.c
> @@ -97,6 +97,8 @@ static int ivf_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt)
>  
>      if (st->codecpar->codec_id == AV_CODEC_ID_VP9)
>          ret = ff_stream_add_bitstream_filter(st, "vp9_superframe", NULL);
> +    else if (st->codecpar->codec_id == AV_CODEC_ID_AV1)
> +        ret = ff_stream_add_bitstream_filter(st, "av1_metadata", "td=insert");
>  
>      return ret;
>  }
> 

I'm not quite seeing why this is wanted - could you explain it a bit further?

(Since IVF is packetised into temporal units already, it seems to me that having TDs or not in the file won't change anything from the point of view of the consumer.)

Thanks,

- Mark
James Almer Oct. 3, 2018, 11:12 p.m.
On 10/3/2018 8:01 PM, Mark Thompson wrote:
> On 03/10/18 01:18, James Almer wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavformat/ivfenc.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c
>> index 66441a2a43..adf72117e9 100644
>> --- a/libavformat/ivfenc.c
>> +++ b/libavformat/ivfenc.c
>> @@ -97,6 +97,8 @@ static int ivf_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt)
>>  
>>      if (st->codecpar->codec_id == AV_CODEC_ID_VP9)
>>          ret = ff_stream_add_bitstream_filter(st, "vp9_superframe", NULL);
>> +    else if (st->codecpar->codec_id == AV_CODEC_ID_AV1)
>> +        ret = ff_stream_add_bitstream_filter(st, "av1_metadata", "td=insert");
>>  
>>      return ret;
>>  }
>>
> 
> I'm not quite seeing why this is wanted - could you explain it a bit further?
> 
> (Since IVF is packetised into temporal units already, it seems to me that having TDs or not in the file won't change anything from the point of view of the consumer.)

No, but ivf afaik doesn't suggest or mandate the removal of TDs, unlike
mp4 and matroska, so i figured it would be best to put them back in
place for such remuxing scenarios.

I don't mind dropping this patch.

> 
> Thanks,
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Mark Thompson Oct. 4, 2018, 10:48 p.m.
On 04/10/18 00:12, James Almer wrote:
> On 10/3/2018 8:01 PM, Mark Thompson wrote:
>> On 03/10/18 01:18, James Almer wrote:
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>  libavformat/ivfenc.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c
>>> index 66441a2a43..adf72117e9 100644
>>> --- a/libavformat/ivfenc.c
>>> +++ b/libavformat/ivfenc.c
>>> @@ -97,6 +97,8 @@ static int ivf_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt)
>>>  
>>>      if (st->codecpar->codec_id == AV_CODEC_ID_VP9)
>>>          ret = ff_stream_add_bitstream_filter(st, "vp9_superframe", NULL);
>>> +    else if (st->codecpar->codec_id == AV_CODEC_ID_AV1)
>>> +        ret = ff_stream_add_bitstream_filter(st, "av1_metadata", "td=insert");
>>>  
>>>      return ret;
>>>  }
>>>
>>
>> I'm not quite seeing why this is wanted - could you explain it a bit further?
>>
>> (Since IVF is packetised into temporal units already, it seems to me that having TDs or not in the file won't change anything from the point of view of the consumer.)
> 
> No, but ivf afaik doesn't suggest or mandate the removal of TDs, unlike
> mp4 and matroska, so i figured it would be best to put them back in
> place for such remuxing scenarios.
> 
> I don't mind dropping this patch.

No preference - I don't have any arguments one way or the other.

If you think it's a good idea then it looks fine to me, though probably the IVF muxer needs to select av1_metadata in configure to match.  (Several muxers seem to be missing these dependencies, including IVF on vp9_superframe - patch incoming.)

Thanks,

- Mark

Patch hide | download patch | download mbox

diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c
index 66441a2a43..adf72117e9 100644
--- a/libavformat/ivfenc.c
+++ b/libavformat/ivfenc.c
@@ -97,6 +97,8 @@  static int ivf_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt)
 
     if (st->codecpar->codec_id == AV_CODEC_ID_VP9)
         ret = ff_stream_add_bitstream_filter(st, "vp9_superframe", NULL);
+    else if (st->codecpar->codec_id == AV_CODEC_ID_AV1)
+        ret = ff_stream_add_bitstream_filter(st, "av1_metadata", "td=insert");
 
     return ret;
 }