[FFmpeg-devel] [PATCH 1/2] opus/matroska: Adding support for DiscardPadding in muxer

[Vignesh Venkatasubramanian]

On Thu, Aug 29, 2013 at 5:29 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Tue, Aug 20, 2013 at 03:13:36PM -0700, Vignesh Venkatasubramanian wrote:
>> Support for end trimming Opus in Matroska is implemented by using
>> the DiscardPadding container element in the Block data. The last
>> chunk is stored as a Block instead of SimpleBlock and the
>> trimming information is stored and used to discard samples that
>> were padded by the Opus codec. This patch adds support for muxing
>> DiscardPadding element into the container with appropriate value.
>> Matroska spec for the DiscardPadding element can be found here:
>> http://matroska.org/technical/specs/index.html#DiscardPadding
>>
>> Signed-off-by: Vignesh Venkatasubramanian <vigneshv at google.com>
>> ---
>>  libavcodec/avpacket.c     |  2 --
>>  libavcodec/libopusenc.c   | 12 ++++++++++++
>>  libavformat/matroska.h    |  1 +
>>  libavformat/matroskaenc.c | 24 ++++++++++++++++++++++--
>>  4 files changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index 7196c31..ac761f1 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -225,8 +225,6 @@ int av_copy_packet_side_data(AVPacket *pkt, AVPacket *src)
>>          int i;
>>          DUP_DATA(pkt->side_data, src->side_data,
>>                  src->side_data_elems * sizeof(*src->side_data), 0, ALLOC_MALLOC);
>> -        memset(pkt->side_data, 0,
>> -                src->side_data_elems * sizeof(*src->side_data));
>>          for (i = 0; i < src->side_data_elems; i++) {
>>              DUP_DATA(pkt->side_data[i].data, src->side_data[i].data,
>>                      src->side_data[i].size, 1, ALLOC_MALLOC);
>
> i see why you need this but simply removing the memset could leave
> the packets in a invalid state on error conditions.
> From a quick look this looks like it could then lead to double frees
>
> Also this belongs in a seperate patch
>

Sorry about that, will refactor into a seperate patch.

>
>
>> diff --git a/libavcodec/libopusenc.c b/libavcodec/libopusenc.c
>> index 59caac7..775b2ef 100644
>> --- a/libavcodec/libopusenc.c
>> +++ b/libavcodec/libopusenc.c
>> @@ -354,6 +354,18 @@ static int libopus_encode(AVCodecContext *avctx, AVPacket *avpkt,
>>      ff_af_queue_remove(&opus->afq, opus->opts.packet_size,
>>                         &avpkt->pts, &avpkt->duration);
>>
>
>> +    if (opus->opts.packet_size - avpkt->duration > 0) {
>
> could the subtraction overflow ?
>

possibly. adding a check for it.

>
>> +        uint8_t* side_data = av_packet_new_side_data(avpkt,
>> +                                                     AV_PKT_DATA_SKIP_SAMPLES,
>> +                                                     10);
>> +        if(side_data == NULL) {
>> +            av_free_packet(avpkt);
>> +            av_free(avpkt);
>> +            return AVERROR(ENOMEM);
>> +        }
>> +        AV_WL32(side_data + 4, opus->opts.packet_size - avpkt->duration);
>> +    }
>> +
>>      *got_packet_ptr = 1;
>>
>>      return 0;
>
> this too belongs in a seperate patch

Sorry again, will refactor.

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> He who knows, does not speak. He who speaks, does not know. -- Lao Tsu



-- 
Vignesh Venkat | Software Engineer | vigneshv at google.com | 650-861-1330