[MPlayer-dev-eng] [PATCH] EDL extensions, resubmit

[Reynaldo H. Verdejo Pinochet]

Hi Vlad

On Sat, Jun 19, 2010 at 01:03:05PM -0400, Vlad Seryakov wrote:
> Hi Reynaldo
> 
> I've made i think all changes you've asked, it was my first patch submit and i missed the guidelines, sorry.

No problem though a reply to the correct thread would have given you extra
points ;)

> The reason i removed edl_seek function because to properly handle in-the-midle-of-the-scene situation i will have to duplicate code to handle reset, and i just moved edl_reset logic with extensions inside edl_update and introduced a flag that triggers reset. Also because reset may trigger another seek it needs to be inside edl_update, i thought about putting reset logic in separate function but then i need return code handling if i triggered a seek and code become more complicated than it needs to be.

Fair enough, although removing the function in a separate patch would
have been better..


> Index: mplayer.c
> ===================================================================
> --- mplayer.c	(revision 31471)
> +++ mplayer.c	(working copy)
> @@ -355,6 +355,8 @@
>  edl_record_ptr edl_records = NULL; ///< EDL entries memory area
>  edl_record_ptr next_edl_record = NULL; ///< only for traversing edl_records
>  short edl_decision = 0; ///< 1 when an EDL operation has been made.
> +short edl_needs_reset = 0; ///< 1 if we need to run edl_seek_reset again
> +short edl_backward = 0; ///< 1 if we need to skip to the beginning of the next edl record

s/edl/EDL

> -    if (mpctx->sh_video->pts >= next_edl_record->start_sec) {
> +    // This indicates that we need to reset next edl record according 

Here too.

> +    // to new PTS due to seek or other condition
> +    if (edl_needs_reset) {
> +	edl_needs_reset = 0;
> +
> +	mpctx->edl_muted = 0;
> +	next_edl_record = edl_records;
> +
> +	// Find next record, also skip immediately if we are already 
> +        // inside any record

Messed up indentation.

> +	while (next_edl_record) {
> +	    if (next_edl_record->start_sec <= mpctx->sh_video->pts && 
> +               	next_edl_record->stop_sec >= mpctx->sh_video->pts) {
> +
> +		if (edl_backward) {

Here too.

> +		    mpctx->osd_function = OSD_REW;
> +		    edl_decision = 1;
> +		    abs_seek_pos = 0;
> +		    rel_seek_secs = -(mpctx->sh_video->pts - 
> +				      next_edl_record->start_sec + 2);

Why two? 

> +		    mp_msg(MSGT_CPLAYER, MSGL_DBG4, "EDL_SKIP: pts [%f], offset [%f], "
> +			   "start [%f], stop [%f], length [%f]\n", 
> +			   mpctx->sh_video->pts, rel_seek_secs, 
> +                           next_edl_record->start_sec, next_edl_record->stop_sec, 
> +			   next_edl_record->length_sec);

Please align.

> +		    return;
> +		}
> +		break;
> +	    }
> +
> +	    if (next_edl_record->start_sec >= mpctx->sh_video->pts)
> +		break;

Move to top of while replacing >= with > and replace previous if
(next_edl_record->start_sec <= mpctx->sh_video->pts &&
next_edl_record->stop_sec >= mpctx->sh_video->pts) by
if(next_edl_record->stop_sec >= mpctx->sh_video->pts) maybe?

> +	    if (next_edl_record->action == EDL_MUTE)
> +		mpctx->edl_muted = !mpctx->edl_muted;
> +	    next_edl_record = next_edl_record->next;
> +	}

Blank line here wont hurt.

> +	if ((mpctx->user_muted | mpctx->edl_muted) != mpctx->mixer.muted)
> +	    mixer_mute(&mpctx->mixer);
> +    }

Messed up indentation.

> +
> +    if (next_edl_record && mpctx->sh_video->pts >= next_edl_record->start_sec) {
>  	if (next_edl_record->action == EDL_SKIP) {
>  	    mpctx->osd_function = OSD_FFW;
> +	    edl_decision = 1;
>  	    abs_seek_pos = 0;
> -	    rel_seek_secs = next_edl_record->length_sec;
> -	    mp_msg(MSGT_CPLAYER, MSGL_DBG4, "EDL_SKIP: start [%f], stop "
> -		   "[%f], length [%f]\n", next_edl_record->start_sec,
> -		   next_edl_record->stop_sec, next_edl_record->length_sec);
> -	    edl_decision = 1;
> +	    rel_seek_secs = next_edl_record->stop_sec - mpctx->sh_video->pts;
> +	    mp_msg(MSGT_CPLAYER, MSGL_DBG4, "EDL_SKIP: pts [%f], offset [%f], "
> +		   "start [%f], stop [%f], length [%f]\n", 
> +		   mpctx->sh_video->pts, rel_seek_secs, 
> +                   next_edl_record->start_sec, next_edl_record->stop_sec, 
> +		   next_edl_record->length_sec);

Please align.

> -    edl_seek_reset(mpctx);
> +    // Delay EDL reset until we have valid PTS, this will be done in 
> +    // edl_update later
> +    if (edl_records) {
> +	edl_needs_reset = 1;
> +	edl_backward = amount < 0;
> +    }

Messed up indentation.

>  
>      c_total = 0;
>      max_pts_correction = 0.1;
> @@ -3873,7 +3903,7 @@
>      frame_time_remaining = sleep_until_update(&mpctx->time_frame, &aq_sleep_time);
>  
>  //====================== FLIP PAGE (VIDEO BLT): =========================
> -

Leave the blank line and please don't mix cosmetics with code changes.

> +if (!edl_needs_reset) {
>          current_module="flip_page";
>          if (!frame_time_remaining && blit_frame) {
>  	   unsigned int t2=GetTimer();
> @@ -3883,6 +3913,7 @@
>  
>  	   vout_time_usage += (GetTimer() - t2) * 0.000001;
>          }
> +}

Messed up indentation.


Thanks for your work.

--
Reynaldo