[FFmpeg-devel] [PATCH] opus/matroska: Adding support for End Trimming in demuxer/decoder

[Vignesh Venkatasubramanian]

On Thu, Sep 5, 2013 at 4:09 PM, Vignesh Venkatasubramanian
<vigneshv at google.com> wrote:
> On Thu, Sep 5, 2013 at 12:33 PM, Nicolas George
> <nicolas.george at normalesup.org> wrote:
>> L'octidi 18 fructidor, an CCXXI, Michael Niedermayer a écrit :
>>> i confirm this fixes the issues
>>>
>>> anyone else has any comments about the patchset ?
>>> nicolas ? you worked with (lib)opus IIRC
>>
>> I worked on the decoder, not the encoder. Assuming I understand correctly
>> what ff_af_queue does, the libopusenc part looks fine (except style:
>> "uint8_t* side_data" -> "uint8_t *side_data" and "side_data == NULL" ->
>> "!side_data").
>>
>> I can not comment usefully on the Matroska part, though. It looks fine at
>> first glance, but I would not spot a subtle mistake.
>>
>> The "(side_data_size && additional_id == 1)" and "(side_data_size &&
>> additional_id == 1) || discard_padding" tests look a bit fragile, and may
>> become a maintenance burden. Maybe checking block_group to see if it was
>> opened
>
> Not sure what you mean by this. But we are opening block_group based on the
> condition which you want to eliminate.
>
>> or setting a flag at the same time would be cleaner.
>
> Setting a flag will mean a lot more refactoring. BlockGroup can arise only in 2
> cases now which are covered by the checks currently. If we need to use flags,
> then we might need more than one flag (one each for different types of case
> that could trigger opening a BlockGroup) to make the refactor worth it. If you
> think it will be cleaner that way, please let me know. I can make the change.
>

Michael/Nicolas, any thoughts on this?

>> But that does
>> not change the logic of the code.
>>
>> Regards,
>>
>> --
>>   Nicolas George
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>