Re: [FFmpeg-devel] [PATCH] libavcodec/ccaption_dec: remove unnecessary buffering of closed caption packets

[Clément Bœsch]

On Fri, Jan 08, 2016 at 05:24:15PM +0530, Anshul wrote:
> 
> 
> On 6 January 2016 1:55:27 am IST, "Clément Bœsch"  wrote:
> >On Sun, Jan 03, 2016 at 01:07:15PM +0100, Clément Bœsch wrote:
> >[...]
> >> This indeed LGTM, but I'm not the maintainer.
> >> 
> >
> >OK I finally understood why it's done that way: validate_cc_data_pair()
> >alters the pkt data, but the decoder isn't supposed to do that.
> >
> >So this patch is actually incorrect in this state.
> 
> 
> I did not knew, what could be the disadvantage of changing pkt data, I
> changed it to save some CPU cycles.

The packet is considered read-only for the decoder. It appears not to be
marked as const for whatever bad reason.

But what CPU cycles would you be saving anyway here? I don't see what
operation you are saving.

[...]

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavcodec/ccaption_dec: remove unnecessary buffering of closed caption packets

[Anshul]

On 8 January 2016 5:27:37 pm IST, "Clément Bœsch"  wrote:
>On Fri, Jan 08, 2016 at 05:24:15PM +0530, Anshul wrote:
>> 
>> 
>> On 6 January 2016 1:55:27 am IST, "Clément Bœsch"  wrote:
>> >On Sun, Jan 03, 2016 at 01:07:15PM +0100, Clément Bœsch wrote:
>> >[...]
>> >> This indeed LGTM, but I'm not the maintainer.
>> >> 
>> >
>> >OK I finally understood why it's done that way:
>validate_cc_data_pair()
>> >alters the pkt data, but the decoder isn't supposed to do that.
>> >
>> >So this patch is actually incorrect in this state.
>> 
>> 
>> I did not knew, what could be the disadvantage of changing pkt data,
>I
>> changed it to save some CPU cycles.
>
>The packet is considered read-only for the decoder. It appears not to
>be
>marked as const for whatever bad reason.
>
>But what CPU cycles would you be saving anyway here? I don't see what
>operation you are saving.
>
>[...]

Not much, just bitwise operations to discard parity bit, every time for 
conversion and comparison.

-Anshul

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavcodec/ccaption_dec: remove unnecessary buffering of closed caption packets

[Anshul]

On 6 January 2016 1:55:27 am IST, "Clément Bœsch"  wrote:
>On Sun, Jan 03, 2016 at 01:07:15PM +0100, Clément Bœsch wrote:
>[...]
>> This indeed LGTM, but I'm not the maintainer.
>> 
>
>OK I finally understood why it's done that way: validate_cc_data_pair()
>alters the pkt data, but the decoder isn't supposed to do that.
>
>So this patch is actually incorrect in this state.


I did not knew, what could be the disadvantage of changing pkt data, I changed 
it to save some CPU cycles.

But I can change it, if there is any disadvantage.

Over there I have just removed parity bit

-Anshul
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavcodec/ccaption_dec: remove unnecessary buffering of closed caption packets

[Clément Bœsch]

On Sun, Jan 03, 2016 at 01:07:15PM +0100, Clément Bœsch wrote:
[...]
> This indeed LGTM, but I'm not the maintainer.
> 

OK I finally understood why it's done that way: validate_cc_data_pair()
alters the pkt data, but the decoder isn't supposed to do that.

So this patch is actually incorrect in this state.

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavcodec/ccaption_dec: remove unnecessary buffering of closed caption packets

[Aman Gupta]

On Tue, Jan 5, 2016 at 12:25 PM, Clément Bœsch  wrote:

> On Sun, Jan 03, 2016 at 01:07:15PM +0100, Clément Bœsch wrote:
> [...]
> > This indeed LGTM, but I'm not the maintainer.
> >
>
> OK I finally understood why it's done that way: validate_cc_data_pair()
> alters the pkt data, but the decoder isn't supposed to do that.
>
> So this patch is actually incorrect in this state.
>

Wow, good catch. The memcpy() makes much more sense now. Thanks.


>
> --
> Clément B.
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavcodec/ccaption_dec: remove unnecessary buffering of closed caption packets

[Clément Bœsch]

On Fri, Jan 01, 2016 at 04:40:06PM -0800, Aman Gupta wrote:
> From: Aman Gupta 
> 
> CC data is fed to in small chunks (usually 60 bytes at a time)
> and is parsed fully by the eia608 decoder. There is no reason to copy it
> into a secondary buffer first.
> ---
>  libavcodec/ccaption_dec.c | 22 +-
>  1 file changed, 1 insertion(+), 21 deletions(-)
> 
> diff --git a/libavcodec/ccaption_dec.c b/libavcodec/ccaption_dec.c
> index 9f67caa..4b1d376 100644
> --- a/libavcodec/ccaption_dec.c
> +++ b/libavcodec/ccaption_dec.c
> @@ -167,8 +167,6 @@ typedef struct CCaptionSubContext {
>  int64_t startv_time;
>  int64_t end_time;
>  char prev_cmd[2];
> -/* buffer to store pkt data */
> -AVBufferRef *pktbuf;
>  }CCaptionSubContext;
>  
>  
> @@ -185,11 +183,6 @@ static av_cold int init_decoder(AVCodecContext *avctx)
>  if(ret < 0) {
>  return ret;
>  }
> -/* allocate pkt buffer */
> -ctx->pktbuf = av_buffer_alloc(128);
> -if( !ctx->pktbuf) {
> -ret = AVERROR(ENOMEM);
> -}
>  return ret;
>  }
>  
> @@ -197,7 +190,6 @@ static av_cold int close_decoder(AVCodecContext *avctx)
>  {
>  CCaptionSubContext *ctx = avctx->priv_data;
>  av_bprint_finalize( >buffer, NULL);
> -av_buffer_unref(>pktbuf);
>  return 0;
>  }
>  
> @@ -524,23 +516,11 @@ static int decode(AVCodecContext *avctx, void *data, 
> int *got_sub, AVPacket *avp
>  {
>  CCaptionSubContext *ctx = avctx->priv_data;
>  AVSubtitle *sub = data;
> -uint8_t *bptr = NULL;
> +uint8_t *bptr = avpkt->data;
>  int len = avpkt->size;
>  int ret = 0;
>  int i;
>  
> -if ( ctx->pktbuf->size < len) {
> -ret = av_buffer_realloc(>pktbuf, len);
> - if(ret < 0) {
> -av_log(ctx, AV_LOG_WARNING, "Insufficient Memory of %d truncated 
> to %d\n",len, ctx->pktbuf->size);
> -len = ctx->pktbuf->size;
> -ret = 0;
> -}
> -}
> -memcpy(ctx->pktbuf->data, avpkt->data, len);
> -bptr = ctx->pktbuf->data;
> -
> -
>  for (i  = 0; i < len; i += 3) {
>  uint8_t cc_type = *(bptr + i) & 3;
>  if (validate_cc_data_pair( bptr + i) )

This indeed LGTM, but I'm not the maintainer.

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] libavcodec/ccaption_dec: remove unnecessary buffering of closed caption packets

[Aman Gupta]

From: Aman Gupta 

CC data is fed to in small chunks (usually 60 bytes at a time)
and is parsed fully by the eia608 decoder. There is no reason to copy it
into a secondary buffer first.
---
 libavcodec/ccaption_dec.c | 22 +-
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/libavcodec/ccaption_dec.c b/libavcodec/ccaption_dec.c
index 9f67caa..4b1d376 100644
--- a/libavcodec/ccaption_dec.c
+++ b/libavcodec/ccaption_dec.c
@@ -167,8 +167,6 @@ typedef struct CCaptionSubContext {
 int64_t startv_time;
 int64_t end_time;
 char prev_cmd[2];
-/* buffer to store pkt data */
-AVBufferRef *pktbuf;
 }CCaptionSubContext;
 
 
@@ -185,11 +183,6 @@ static av_cold int init_decoder(AVCodecContext *avctx)
 if(ret < 0) {
 return ret;
 }
-/* allocate pkt buffer */
-ctx->pktbuf = av_buffer_alloc(128);
-if( !ctx->pktbuf) {
-ret = AVERROR(ENOMEM);
-}
 return ret;
 }
 
@@ -197,7 +190,6 @@ static av_cold int close_decoder(AVCodecContext *avctx)
 {
 CCaptionSubContext *ctx = avctx->priv_data;
 av_bprint_finalize( >buffer, NULL);
-av_buffer_unref(>pktbuf);
 return 0;
 }
 
@@ -524,23 +516,11 @@ static int decode(AVCodecContext *avctx, void *data, int 
*got_sub, AVPacket *avp
 {
 CCaptionSubContext *ctx = avctx->priv_data;
 AVSubtitle *sub = data;
-uint8_t *bptr = NULL;
+uint8_t *bptr = avpkt->data;
 int len = avpkt->size;
 int ret = 0;
 int i;
 
-if ( ctx->pktbuf->size < len) {
-ret = av_buffer_realloc(>pktbuf, len);
- if(ret < 0) {
-av_log(ctx, AV_LOG_WARNING, "Insufficient Memory of %d truncated 
to %d\n",len, ctx->pktbuf->size);
-len = ctx->pktbuf->size;
-ret = 0;
-}
-}
-memcpy(ctx->pktbuf->data, avpkt->data, len);
-bptr = ctx->pktbuf->data;
-
-
 for (i  = 0; i < len; i += 3) {
 uint8_t cc_type = *(bptr + i) & 3;
 if (validate_cc_data_pair( bptr + i) )
-- 
2.5.3

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel