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

Vignesh Venkatasubramanian vigneshv at google.com
Fri Sep 6 01:09:42 CEST 2013


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.

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


More information about the ffmpeg-devel mailing list