[FFmpeg-devel] [PATCH] forward ASF tags to RTSP for MS-RTSP

Ronald S. Bultje rsbultje
Fri Jan 14 21:03:27 CET 2011


Hi,

On Thu, Jan 13, 2011 at 11:36 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> On Thu, Jan 13, 2011 at 6:33 AM, Aurelien Jacobs <aurel at gnuage.org> wrote:
>> On Wed, Jan 12, 2011 at 10:55:08PM -0500, Ronald S. Bultje wrote:
>>> On Wed, Jan 12, 2011 at 8:07 AM, Aurelien Jacobs <aurel at gnuage.org> wrote:
>>> > On Tue, Jan 11, 2011 at 10:39:37PM -0500, Ronald S. Bultje wrote:
>>> >> On Mon, Jan 3, 2011 at 6:48 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
>>> >> > On Mon, Jan 03, 2011 at 03:06:20PM -0500, Justin Ruggles wrote:
>>> >> >> On 01/03/2011 01:22 PM, Ronald S. Bultje wrote:
>>> >> >>
>>> >> >> > Hi,
>>> >> >> >
>>> >> >> > On Mon, Jan 3, 2011 at 1:17 PM, Martin Storsj? <martin at martin.st> wrote:
>>> >> >> >> On Mon, 3 Jan 2011, Ronald S. Bultje wrote:
>>> >> >> >>
>>> >> >> >>> for chained demuxers, we need to manually FW tags, this patch does it
>>> >> >> >>> for MS-RTSP. Fixes issue 2478.
>>> >> >> >>
>>> >> >> >> Looks good to me.
>>> >> >> >>
>>> >> >> >> As future improvment - should we make this into a reusable function? I
>>> >> >> >> guess something similar might be needed for other
>>> >> >> >> full-container-within-another-container setups (like mpegts in RTP).
>>> >> >> >
>>> >> >> > Sounds like a good plan - but I'd like to leave that question to other
>>> >> >> > metadata experts. Anton? Aurel? Baptiste? Michael?
>>> >> >>
>>> >> >>
>>> >> >> search ffmpeg.c for "autocopy" and you'll find lots of code very similar
>>> >> >> to this. ?So adding it to the public API might be a good idea.
>>> >> >
>>> >> > I agree that a public metadata_copy API would be useful.
>>> >> > Something like:
>>> >> > ? ?int av_metadata_copy(AVMetadata **dest, AVMetadata *src);
>>> >>
>>> >> Has anyone implemented this since then?
>>> >
>>> > Not that I know of, but IIRC there was some patches sent here a long
>>> > time ago proposing something similar (and never accepted).
>>> >
>>> >> Or should I do it myself?
>>> >
>>> > This shouldn't be hard to do, so I would say yes, go ahead.
>>>
>>> See attached. If wanted I can apply in two steps. I called in ff_*()
>>> because we may want to do stuff before making it public (but then
>>> again, ffmpeg.c uses it so maybe it should be public, in that case
>>> please comment on API also.)
>>
>> IMHO it is mandatory to make it public to use it in ffmpeg.c.
>
> Fair enough, locally renamed to av_metadata_copy() then.
>
>> Overall, patch looks OK to me.
>>
>>> Index: libavformat/metadata.c
>>> ===================================================================
>>> --- libavformat/metadata.c ? ?(revision 26130)
>>> +++ libavformat/metadata.c ? ?(working copy)
>>> @@ -158,3 +158,11 @@
>>> ? ? ?for (i=0; i<ctx->nb_programs; i++)
>>> ? ? ? ? ?ff_metadata_conv(&ctx->programs[i]->metadata, d_conv, s_conv);
>>> ?}
>>> +
>>> +void ff_metadata_copy(AVMetadata **dst, AVMetadata *src, int dst_flags)
>>> +{
>>> + ? ?AVMetadataTag *t = NULL;
>>> +
>>> + ? ?while ((t = av_metadata_get(src, "", t, AV_METADATA_IGNORE_SUFFIX)))
>>> + ? ? ? ?av_metadata_set2(dst, t->key, t->value, dst_flags);
>>> +}
>>
>> I would personnaly rename 'dst_flags' to 'flags' but that's just
>> unimportant bikeshed.
>
> No problem, will change.
>
>>> Index: libavformat/rtpdec_asf.c
>>> ===================================================================
>>> --- libavformat/rtpdec_asf.c ?(revision 26130)
>>> +++ libavformat/rtpdec_asf.c ?(working copy)
>>> @@ -112,6 +112,7 @@
>>> ? ? ? ? ?ret = av_open_input_stream(&rt->asf_ctx, &pb, "", &asf_demuxer, NULL);
>>> ? ? ? ? ?if (ret < 0)
>>> ? ? ? ? ? ? ?return ret;
>>> + ? ? ? ?ff_metadata_copy(&s->metadata, rt->asf_ctx->metadata, 0);
>>> ? ? ? ? ?rt->asf_pb_pos = url_ftell(&pb);
>>> ? ? ? ? ?av_free(buf);
>>> ? ? ? ? ?rt->asf_ctx->pb = NULL;
>>
>> IMHO this should be committed separately.
>
> Will do. Anyone else have additional comments or can I commit? No
> comments tonight = I will apply.

Applied.

Ronald



More information about the ffmpeg-devel mailing list