On Mon, Oct 17, 2016 at 01:47:32PM -0400, Vittorio Giovara wrote: > On Mon, Oct 17, 2016 at 1:30 PM, Diego Biurrun <di...@biurrun.de> wrote: > > Looking at this in more detail now that the first round of review is over. > > > > On Mon, Oct 17, 2016 at 11:54:04AM -0400, Vittorio Giovara wrote: > >> --- a/tests/fate-run.sh > >> +++ b/tests/fate-run.sh > >> @@ -76,6 +76,11 @@ probefmt(){ > >> > >> +probear(){ > >> + run avprobe -show_stream_entry sample_aspect_ratio -v 0 "$@" > >> + run avprobe -show_stream_entry display_aspect_ratio -v 0 "$@" > >> +} > >> + > >> --- a/tests/fate/probe.mak > >> +++ b/tests/fate/probe.mak > >> @@ -16,3 +16,16 @@ fate-probe-format: $(FATE_PROBE_FORMAT) > >> $(FATE_PROBE_FORMAT): avprobe$(EXESUF) > >> +$(FATE_MOV): avprobe$(EXESUF) > > > > You are duplicating the dependency list, don't. > > How do you mean? Is it enough to do > $(FATE_PROBE_FORMAT) += $(FATE _MOV)
$(FATE_PROBE_FORMAT) $(FATE_MOV): avprobe$(EXESUF) > >> +FATE_MOV += fate-mov-display-matrix > >> +fate-mov-display-matrix: CMD = run avprobe -v 0 -show_stream_entry matrix > >> $(TARGET_SAMPLES)/mov/displaymatrix.mov > >> + > >> +FATE_MOV += fate-mov-rotation > >> +fate-mov-rotation: CMD = run avprobe -v 0 -show_stream_entry rotation > >> $(TARGET_SAMPLES)/mov/displaymatrix.mov > >> + > >> +FATE_MOV += fate-mov-ar > >> +fate-mov-ar: CMD = probear $(TARGET_SAMPLES)/mov/displaymatrix.mov > > > > This is inconsistent. You added a convenience function for the last > > test, but not for the first two. You could simply run the last test > > "manually" and then separate the tests for sar and dar - and separating > > tests is always a good thing IMO. Or you could add a convenience > > function that can be used in all tests. > > I know it's inconsistent, but I moved the AR tests in a separate > function because I thought it might usable in other tests, while > rotation and displaymatrix are something that are meaningful only for > mov. Just create one helper function for -show_stream_entry. Diego _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel